RE: [PATCH v2 2/2] target/hexagon: add enums for event, cause
> -Original Message- > From: Brian Cain > Sent: Monday, August 26, 2024 6:27 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com > Subject: [PATCH v2 2/2] target/hexagon: add enums for event, cause > > Signed-off-by: Brian Cain > --- > target/hexagon/cpu_bits.h | 22 ++ > 1 file changed, 14 insertions(+), 8 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH v2 1/2] target/hexagon: rename HEX_EXCP_*=>HEX_CAUSE_*
> -Original Message- > From: Brian Cain > Sent: Monday, August 26, 2024 6:27 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com; Laurent Vivier > Subject: [PATCH v2 1/2] target/hexagon: rename > HEX_EXCP_*=>HEX_CAUSE_* > > The values previously used for "HEX_EXCP_*" were the cause code > definitions and not the event numbers. So in this commit, we update the > names to reflect the cause codes. In HEX_EVENT_TRAP0's case, we add a > new "HEX_EVENT_*" with the correct event number. > > Signed-off-by: Brian Cain > --- > linux-user/hexagon/cpu_loop.c | 4 ++-- > target/hexagon/cpu.h | 2 +- > target/hexagon/cpu_bits.h | 15 --- > target/hexagon/gen_tcg.h | 2 +- > target/hexagon/translate.c| 6 +++--- > 5 files changed, 15 insertions(+), 14 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH 3/3] target/hexagon: add enums for event, cause
> -Original Message- > From: Brian Cain > Sent: Friday, August 16, 2024 1:06 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com > Subject: [PATCH 3/3] target/hexagon: add enums for event, cause > > Signed-off-by: Brian Cain > --- > target/hexagon/cpu_bits.h | 22 ++ > 1 file changed, 14 insertions(+), 8 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH 1/3] target/hexagon: Rename HEX_EXCP_ => HEX_EVENT_
> -Original Message- > From: Brian Cain > Sent: Friday, August 16, 2024 1:06 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com; Laurent Vivier > Subject: [PATCH 1/3] target/hexagon: Rename HEX_EXCP_ => HEX_EVENT_ > > Change the name of these definitions to reflect that they correspond to the > event code for the exception. > > Signed-off-by: Brian Cain > --- > linux-user/hexagon/cpu_loop.c | 4 ++-- > target/hexagon/cpu.h | 2 +- > target/hexagon/cpu_bits.h | 14 +++--- > target/hexagon/gen_tcg.h | 2 +- > target/hexagon/translate.c| 6 +++--- > 5 files changed, 14 insertions(+), 14 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH 2/3] target/hexagon: rename HEX_EVENT_TRAP0=>HEX_CAUSE_TRAP0
> -Original Message- > From: Brian Cain > Sent: Friday, August 16, 2024 1:06 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com > Subject: [PATCH 2/3] target/hexagon: rename > HEX_EVENT_TRAP0=>HEX_CAUSE_TRAP0 > > The value previously used for "HEX_EVENT_TRAP0" was the cause code > definition and not the event number. So in this commit, we update the > name to reflect the cause code and add a new "HEX_EVENT_TRAP0" > with the correct event number. > > Signed-off-by: Brian Cain > --- > target/hexagon/cpu_bits.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Taylor Simpson
RE: [PATCH] target/hexagon: switch to dc set_props() list
> -Original Message- > From: Brian Cain > Sent: Tuesday, July 30, 2024 7:13 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com > Subject: [PATCH] target/hexagon: switch to dc set_props() list > > Define a hexagon_cpu_properties list to match the idiom used by other > targets. > > Signed-off-by: Brian Cain > --- > target/hexagon/cpu.c | 18 -- > 1 file changed, 8 insertions(+), 10 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH] target/hexagon: define a v66 CPU
> -Original Message- > From: Brian Cain > Sent: Tuesday, July 30, 2024 7:10 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; quic_mathb...@quicinc.com; > sidn...@quicinc.com; quic_mlie...@quicinc.com; > ltaylorsimp...@gmail.com > Subject: [PATCH] target/hexagon: define a v66 CPU > > For now, v66 behavior is the same as other CPUs. > > Signed-off-by: Brian Cain > --- > target/hexagon/cpu-qom.h | 1 + > target/hexagon/cpu.c | 2 ++ > 2 files changed, 3 insertions(+) Reviewed-by: Taylor Simpson
RE: [PATCH] Hexagon: fix F2_conv_* instructions for negative zero
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Sunday, July 28, 2024 11:16 AM > To: qemu-devel@nongnu.org > Cc: ltaylorsimp...@gmail.com; bc...@quicinc.com; sidn...@quicinc.com; > a...@rev.ng; a...@rev.ng > Subject: [PATCH] Hexagon: fix F2_conv_* instructions for negative zero > > The implementation for these instructions handles -0 as an invalid float point > value, whereas the Hexagon hardware considers it the same as +0 (which is > valid). Let's fix that and add a regression test. > > Signed-off-by: Matheus Tavares Bernardino > --- > target/hexagon/op_helper.c | 16 > tests/tcg/hexagon/usr.c| 10 ++ > 2 files changed, 18 insertions(+), 8 deletions(-) You should update the copyright year to 2024 in the files you changed. Otherwise Reviewed-by: Taylor Simpson
RE: [PATCH] Hexagon: lldb read/write predicate registers p0/p1/p2/p3
> -Original Message-
> From: Ted Woodward
> Sent: Thursday, June 13, 2024 9:03 AM
> To: ltaylorsimp...@gmail.com; Matheus Bernardino (QUIC)
>
> Cc: qemu-devel@nongnu.org; Brian Cain ;
> alex.ben...@linaro.org; Sid Manning ; Marco
> Liebel (QUIC) ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [PATCH] Hexagon: lldb read/write predicate registers
> p0/p1/p2/p3
>
>
>
> > -Original Message-
> > From: ltaylorsimp...@gmail.com
> > Sent: Wednesday, June 12, 2024 9:49 PM
> > To: Matheus Bernardino (QUIC)
> > Cc: qemu-devel@nongnu.org; Brian Cain ; Ted
> > Woodward ; alex.ben...@linaro.org; Sid
> Manning
> > ; Marco Liebel (QUIC)
> ;
> > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > a...@rev.ng
> > Subject: RE: [PATCH] Hexagon: lldb read/write predicate registers
> > p0/p1/p2/p3
> >
> > > -Original Message-
> > > From: Matheus Tavares Bernardino
> > > Sent: Wednesday, June 12, 2024 12:30 PM
> > > To: ltaylorsimp...@gmail.com
> > > Cc: qemu-devel@nongnu.org; bc...@quicinc.com;
> tedw...@quicinc.com;
> > > alex.ben...@linaro.org; quic_mathb...@quicinc.com;
> > > sidn...@quicinc.com; quic_mlie...@quicinc.com;
> > > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > > a...@rev.ng
> > > Subject: Re: [PATCH] Hexagon: lldb read/write predicate registers
> > > p0/p1/p2/p3
> > >
> > > On Wed, 12 Jun 2024 10:42:39 -0600 Taylor Simpson
> > > wrote:
> > > >
> > > > diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
> > > > index
> > > > 502c6987f0..e67e627fc9 100644
> > > > --- a/target/hexagon/gdbstub.c
> > > > +++ b/target/hexagon/gdbstub.c
> > > > @@ -56,6 +64,15 @@ int hexagon_gdb_write_register(CPUState *cs,
> > > uint8_t *mem_buf, int n)
> > > > return sizeof(target_ulong);
> > > > }
> > > >
> > > > +n -= TOTAL_PER_THREAD_REGS;
> > > > +
> > > > +if (n < NUM_PREGS) {
> > > > +env->pred[n] = ldtul_p(mem_buf);
> > > > +return sizeof(uint8_t);
> > >
> > > I wonder, shouldn't this be sizeof(target_ulong) since we wrote a
> > > target_ulong?
> >
> > Good question.
> >
> > From the architecture point of view, predicates are 8 bits (Section
> > 2.2.5 of the
> > v73 Hexagon PRM). However, we model them in QEMU as target_ulong
> > because TCG variables must be either 32 bits or 64 bits. There isn't
> > an option for 8 bits. Whenever we write to a predicate, do "and" with
> > 0xff first to ensure there are only 8 bits written (see
> > gen_log_pred_write in target/hexagon/genptr.c).
> >
> > I did some more digging and here is what I found:
> > - Since we have bitsize="8" in hexagon-core.xml, lldb will reject any
> > attempt to write something larger.
> > (lldb) reg write p1 0x1ff
> > error: Failed to write register 'p1' with value '0x1ff': value 0x1ff
> > is too large to fit in a 1 byte unsigned integer value
> > - For the lldb "reg write" command, the return value from
> > hexagon_gdb_write_register isn't used.
> > - The only place the return value is used is in handle_write_all_regs.
> > This function is called in response to a "G" packet from the debugger.
> > I don't know if/when lldb uses this packet, but it seems like it would
> > count on it being 8 bits since that's what is in hexagon-core.xml.
> >
> > Ted , when would lldb generate a "G" packet, and
> > what assumptions will it make about the size of predicate registers?
>
> When you use the expression parser to call a function, lldb will save the
> current state, set up the function call, set a breakpoint on a return (by
> changing the lr register and setting a breakpoint on the new address), set
the
> PC to the function address, and resume. After the breakpoint is hit, lldb
will
> restore the saved state.
>
> Since QEMU doesn't support the lldb RSP extension
> QSaveRegisterState/QRestoreRegisterState,
> lldb will use G/g packets to save and restore the register state.
>
> lldb doesn't interpret the values from the G/g packets. It just saves and
> restores them, so I don't think the new predicate definitions will matter
for
> that. You can test this out by changing the predicate registers, then
calling a
> function with the expression parser. Not a varargs function, since the IR
> interpreter doesn't handle those.
>
> Ted
Thanks Ted! We do indeed execute handle_write_all_regs when we print the
result of a function call in lldb.
So, the answer to Metheus' question is "no". We should return sizeof
uint8_t. However, we should also mask off the high bits from the value
returned from ldtul_p before assigning to the predicate register. This
avoids putting bits from subsequent items in the buffer into the register.
env->pred[n] = ldtul_p(mem_buf) & 0xff;
I'll send v2 of the patch with this change shortly.
Taylor
RE: [PATCH] Hexagon: lldb read/write predicate registers p0/p1/p2/p3
> -Original Message-
> From: Matheus Tavares Bernardino
> Sent: Wednesday, June 12, 2024 12:30 PM
> To: ltaylorsimp...@gmail.com
> Cc: qemu-devel@nongnu.org; bc...@quicinc.com; tedw...@quicinc.com;
> alex.ben...@linaro.org; quic_mathb...@quicinc.com;
> sidn...@quicinc.com; quic_mlie...@quicinc.com;
> richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: Re: [PATCH] Hexagon: lldb read/write predicate registers
> p0/p1/p2/p3
>
> On Wed, 12 Jun 2024 10:42:39 -0600 Taylor Simpson
> wrote:
> >
> > diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c index
> > 502c6987f0..e67e627fc9 100644
> > --- a/target/hexagon/gdbstub.c
> > +++ b/target/hexagon/gdbstub.c
> > @@ -56,6 +64,15 @@ int hexagon_gdb_write_register(CPUState *cs,
> uint8_t *mem_buf, int n)
> > return sizeof(target_ulong);
> > }
> >
> > +n -= TOTAL_PER_THREAD_REGS;
> > +
> > +if (n < NUM_PREGS) {
> > +env->pred[n] = ldtul_p(mem_buf);
> > +return sizeof(uint8_t);
>
> I wonder, shouldn't this be sizeof(target_ulong) since we wrote a
> target_ulong?
Good question.
>From the architecture point of view, predicates are 8 bits (Section 2.2.5 of
the v73 Hexagon PRM). However, we model them in QEMU as target_ulong
because TCG variables must be either 32 bits or 64 bits. There isn't an
option for 8 bits. Whenever we write to a predicate, do "and" with 0xff
first to ensure there are only 8 bits written (see gen_log_pred_write in
target/hexagon/genptr.c).
I did some more digging and here is what I found:
- Since we have bitsize="8" in hexagon-core.xml, lldb will reject any
attempt to write something larger.
(lldb) reg write p1 0x1ff
error: Failed to write register 'p1' with value '0x1ff': value 0x1ff is
too large to fit in a 1 byte unsigned integer value
- For the lldb "reg write" command, the return value from
hexagon_gdb_write_register isn't used.
- The only place the return value is used is in handle_write_all_regs. This
function is called in response to a "G" packet from the debugger. I don't
know if/when lldb uses this packet, but it seems like it would count on it
being 8 bits since that's what is in hexagon-core.xml.
Ted , when would lldb generate a "G" packet, and what
assumptions will it make about the size of predicate registers?
Thanks,
Taylor
RE: [PATCH v2 3/4] target/hexagon: idef-parser fix leak of init_list
> -Original Message- > From: Anton Johansson > Sent: Friday, May 10, 2024 9:53 AM > To: qemu-devel@nongnu.org > Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com > Subject: [PATCH v2 3/4] target/hexagon: idef-parser fix leak of init_list > > gen_inst_init_args() is called for instructions using a predicate as an rvalue. > Upon first call, the list of arguments which might need initialization init_list is > freed to indicate that they have been processed. For instructions without an > rvalue predicate, > gen_inst_init_args() isn't called and init_list will never be freed. > > Free init_list from free_instruction() if it hasn't already been freed. > A comment in free_instruction is also updated. > > Signed-off-by: Anton Johansson Reviewed-by: Taylor Simpson
RE: [PATCH v2 4/4] target/hexagon: idef-parser simplify predicate init
> -Original Message- > From: Anton Johansson > Sent: Friday, May 10, 2024 9:53 AM > To: qemu-devel@nongnu.org > Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com > Subject: [PATCH v2 4/4] target/hexagon: idef-parser simplify predicate init > > Only predicate instruction arguments need to be initialized by idef-parser. > This commit removes registers from the init_list and simplifies > gen_inst_init_args() slightly. > > Signed-off-by: Anton Johansson Reviewed-by: Taylor Simpson
RE: [PATCH] Hexagon: fix HVX store new
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Monday, May 20, 2024 10:53 AM > To: qemu-devel@nongnu.org > Cc: ltaylorsimp...@gmail.com; sidn...@quicinc.com; bc...@quicinc.com; > richard.hender...@linaro.org; a...@rev.ng; a...@rev.ng > Subject: [PATCH] Hexagon: fix HVX store new > > At 09a7e7db0f (Hexagon (target/hexagon) Remove uses of > op_regs_generated.h.inc, 2024-03-06), we've changed the logic of > check_new_value() to use the new pre-calculated > packet->insn[...].dest_idx instead of calculating the index on the fly > using opcode_reginfo[...]. The dest_idx index is calculated roughly like the > following: > > for reg in iset[tag]["syntax"]: > if reg.is_written(): > dest_idx = regno > break > > Thus, we take the first register that is writtable. Before that, however, we > also used to follow an alphabetical order on the register > type: 'd', 'e', 'x', and 'y'. No longer following that makes us select the wrong > register index and the HVX store new instruction does not update the > memory like expected. > > Signed-off-by: Matheus Tavares Bernardino Reviewed-by: Taylor Simpson
RE: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
> -Original Message-
> From: 'Anton Johansson'
> Sent: Tuesday, May 7, 2024 4:47 AM
> To: ltaylorsimp...@gmail.com
> Cc: qemu-devel@nongnu.org; a...@rev.ng; bc...@quicinc.com
> Subject: Re: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
>
> On 06/05/24, ltaylorsimp...@gmail.com wrote:
> >
> >
> > > -Original Message-
> > > From: Anton Johansson
> > > Sent: Monday, May 6, 2024 1:31 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com
> > > Subject: [PATCH 3/4] target/hexagon: idef-parser fix leak of
> > > init_list
> > >
> > > gen_inst_init_args() is called for instructions using a predicate as
> > > an
> > rvalue.
> > > Upon first call, the list of arguments which might need
> > > initialization
> > init_list is
> > > freed to indicate that they have been processed. For instructions
> > > without
> > an
> > > rvalue predicate,
> > > gen_inst_init_args() isn't called and init_list will never be freed.
> > >
> > > Free init_list from free_instruction() if it hasn't already been freed.
> > >
> > > Signed-off-by: Anton Johansson
> > > ---
> > > target/hexagon/idef-parser/parser-helpers.c | 7 +++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/target/hexagon/idef-parser/parser-helpers.c
> > > b/target/hexagon/idef-parser/parser-helpers.c
> > > index 95f2b43076..bae01c2bb8 100644
> > > --- a/target/hexagon/idef-parser/parser-helpers.c
> > > +++ b/target/hexagon/idef-parser/parser-helpers.c
> > > @@ -2121,6 +2121,13 @@ void free_instruction(Context *c)
> > > g_string_free(g_array_index(c->inst.strings, GString*, i), TRUE);
> > > }
> > > g_array_free(c->inst.strings, TRUE);
> > > +/*
> > > + * Free list of arguments that might need initialization, if
> > > + they
> > haven't
> > > + * already been free'd.
> > > + */
> > > +if (c->inst.init_list) {
> > > +g_array_free(c->inst.init_list, TRUE);
> > > +}
> > > /* Free INAME token value */
> > > g_string_free(c->inst.name, TRUE);
> > > /* Free variables and registers */
> >
> > Why not do this in gen_inst_init_args just before this?
> >/* Free argument init list once we have initialized everything */
> > g_array_free(c->inst.init_list, TRUE);
> > c->inst.init_list = NULL;
>
> Thanks for the reviews Taylor! I'm not sure I understand what you mean
> here, we already free init_list in gen_inst_init_args, since
> gen_inst_init_args
> won't be called for all instructions we need to also free from a common
> place.
>
> //Anton
It just seems more natural to free the elements of the array at the same place
you free the array itself. If there are valid reasons for doing it elsewhere,
I'm OK with that.
Taylor
RE: [PATCH 2/4] target/hexagon: idef-parser remove undefined functions
> -Original Message- > From: Anton Johansson > Sent: Monday, May 6, 2024 1:31 PM > To: qemu-devel@nongnu.org > Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com > Subject: [PATCH 2/4] target/hexagon: idef-parser remove undefined > functions > > Signed-off-by: Anton Johansson > --- > target/hexagon/idef-parser/parser-helpers.h | 13 - > 1 file changed, 13 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
> -Original Message-
> From: Anton Johansson
> Sent: Monday, May 6, 2024 1:31 PM
> To: qemu-devel@nongnu.org
> Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com
> Subject: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
>
> gen_inst_init_args() is called for instructions using a predicate as an
rvalue.
> Upon first call, the list of arguments which might need initialization
init_list is
> freed to indicate that they have been processed. For instructions without
an
> rvalue predicate,
> gen_inst_init_args() isn't called and init_list will never be freed.
>
> Free init_list from free_instruction() if it hasn't already been freed.
>
> Signed-off-by: Anton Johansson
> ---
> target/hexagon/idef-parser/parser-helpers.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/target/hexagon/idef-parser/parser-helpers.c
> b/target/hexagon/idef-parser/parser-helpers.c
> index 95f2b43076..bae01c2bb8 100644
> --- a/target/hexagon/idef-parser/parser-helpers.c
> +++ b/target/hexagon/idef-parser/parser-helpers.c
> @@ -2121,6 +2121,13 @@ void free_instruction(Context *c)
> g_string_free(g_array_index(c->inst.strings, GString*, i), TRUE);
> }
> g_array_free(c->inst.strings, TRUE);
> +/*
> + * Free list of arguments that might need initialization, if they
haven't
> + * already been free'd.
> + */
> +if (c->inst.init_list) {
> +g_array_free(c->inst.init_list, TRUE);
> +}
> /* Free INAME token value */
> g_string_free(c->inst.name, TRUE);
> /* Free variables and registers */
Why not do this in gen_inst_init_args just before this?
/* Free argument init list once we have initialized everything */
g_array_free(c->inst.init_list, TRUE);
c->inst.init_list = NULL;
Taylor
RE: [PATCH 4/4] target/hexagon: idef-parser simplify predicate init
> -Original Message-
> From: Anton Johansson
> Sent: Monday, May 6, 2024 1:31 PM
> To: qemu-devel@nongnu.org
> Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com
> Subject: [PATCH 4/4] target/hexagon: idef-parser simplify predicate init
>
> Only predicate instruction arguments need to be initialized by
idef-parser.
> This commit removes registers from the init_list and simplifies
> gen_inst_init_args() slightly.
>
> Signed-off-by: Anton Johansson
> ---
> target/hexagon/idef-parser/idef-parser.y| 2 --
> target/hexagon/idef-parser/parser-helpers.c | 20 +---
> 2 files changed, 9 insertions(+), 13 deletions(-)
> diff --git a/target/hexagon/idef-parser/parser-helpers.c
> b/target/hexagon/idef-parser/parser-helpers.c
> index bae01c2bb8..33e8f82007 100644
> --- a/target/hexagon/idef-parser/parser-helpers.c
> +++ b/target/hexagon/idef-parser/parser-helpers.c
> @@ -1652,26 +1652,24 @@ void gen_inst(Context *c, GString *iname)
>
> void gen_inst_init_args(Context *c, YYLTYPE *locp) {
> +/* If init_list is NULL arguments have already been initialized */
> if (!c->inst.init_list) {
> return;
> }
>
> for (unsigned i = 0; i < c->inst.init_list->len; i++) {
> HexValue *val = &g_array_index(c->inst.init_list, HexValue, i);
> -if (val->type == REGISTER_ARG) {
> -/* Nothing to do here */
> -} else if (val->type == PREDICATE) {
> -char suffix = val->is_dotnew ? 'N' : 'V';
> -EMIT_HEAD(c, "tcg_gen_movi_i%u(P%c%c, 0);\n", val->bit_width,
> - val->pred.id, suffix);
> -} else {
> -yyassert(c, locp, false, "Invalid arg type!");
> -}
> +yyassert(c, locp, val->type == PREDICATE,
> + "Only predicates need to be initialized!");
> +char suffix = val->is_dotnew ? 'N' : 'V';
Declarations should be at the beginning of the function per QEMU coding
standards.
> +EMIT_HEAD(c, "tcg_gen_movi_i%u(P%c%c, 0);\n", val->bit_width,
Since you know this is a predicate, the bit_width will always be 32. You
can hard-code that instead of using %u.
> + val->pred.id, suffix);
> }
>
> /* Free argument init list once we have initialized everything */
Taylor
RE: [PATCH 1/4] target/hexagon: idef-parser remove unused defines
> -Original Message- > From: Anton Johansson > Sent: Monday, May 6, 2024 1:31 PM > To: qemu-devel@nongnu.org > Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com > Subject: [PATCH 1/4] target/hexagon: idef-parser remove unused defines > > Before switching to GArray/g_string_printf we used fixed size arrays for > output buffers and instructions arguments among other things. > > Macros defining the sizes of these buffers were left behind, remove them. > > Signed-off-by: Anton Johansson > --- > target/hexagon/idef-parser/idef-parser.h | 10 -- > 1 file changed, 10 deletions(-) Reviewed-by: Taylor Simpson
RE: [PATCH v4] Hexagon: add PC alignment check and exception
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Thursday, May 2, 2024 9:55 AM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; sidn...@quicinc.com; a...@rev.ng; a...@rev.ng; > ltaylorsimp...@gmail.com; richard.hender...@linaro.org; Laurent Vivier > > Subject: [PATCH v4] Hexagon: add PC alignment check and exception > > The Hexagon Programmer's Reference Manual says that the exception 0x1e > should be raised upon an unaligned program counter. Let's implement that > and also add some tests. > > Signed-off-by: Matheus Tavares Bernardino > Reviewed-by: Richard Henderson > --- > v3: https://lore.kernel.org/qemu- > devel/5c90567ec28723865e144f386b36f5b676b7a5d3.1714486874.git.quic_ma > thb...@quicinc.com/ > > Changes in v4: > - Added missing regs to clobber list as mentioned by Taylor. > - Avoided undefined behavior on package with multiple branches > (at test_multi_cof), as suggested offline by Brian. > > a/tests/tcg/hexagon/unaligned_pc_multi_cof.S > b/tests/tcg/hexagon/unaligned_pc_multi_cof.S > new file mode 100644 > index 00..10accd0057 > --- /dev/null > +++ b/tests/tcg/hexagon/unaligned_pc_multi_cof.S > @@ -0,0 +1,5 @@ > +.org 0x3 > +.global test_multi_cof_unaligned > +test_multi_cof_unaligned: > + nop > + jumpr r31 You should be able to put this as an inline asm block with the .org directive in unaligned_pc.c (outside of any function). Otherwise Reviewed-by: Taylor Simpson
RE: [PATCH v3] Hexagon: add PC alignment check and exception
> -Original Message-
> From: Matheus Tavares Bernardino
> Sent: Tuesday, April 30, 2024 9:25 AM
> To: qemu-devel@nongnu.org
> Cc: bc...@quicinc.com; sidn...@quicinc.com; a...@rev.ng; a...@rev.ng;
> ltaylorsimp...@gmail.com; richard.hender...@linaro.org; Laurent Vivier
>
> Subject: [PATCH v3] Hexagon: add PC alignment check and exception
>
> The Hexagon Programmer's Reference Manual says that the exception 0x1e
> should be raised upon an unaligned program counter. Let's implement that
> and also add some tests.
>
> Signed-off-by: Matheus Tavares Bernardino
> ---
> v2: https://lore.kernel.org/qemu-
> devel/e559b521d1920f804df10244c8c07564431aeba5.1714419461.git.quic_ma
> thb...@quicinc.com/
>
> Thanks for the comments, Richard and Taylor!
>
> Changed in v3:
> - Removed now unnecessary pkt_raises_exception addition.
> - Added HEX_EXCP_PC_NOT_ALIGNED handling at
> linux-user/hexagon/cpu_loop.c.
> - Merged all tests into a C file that uses signal handler to check
> that the exception was raised.
>
> target/hexagon/cpu.h | 7 ++
> target/hexagon/cpu_bits.h | 4 +
> target/hexagon/macros.h| 3 -
> linux-user/hexagon/cpu_loop.c | 4 +
> target/hexagon/op_helper.c | 9 +--
> tests/tcg/hexagon/unaligned_pc.c | 85 ++
> tests/tcg/hexagon/Makefile.target | 4 +
> tests/tcg/hexagon/unaligned_pc_multi_cof.S | 5 ++
> 8 files changed, 113 insertions(+), 8 deletions(-) create mode 100644
> tests/tcg/hexagon/unaligned_pc.c create mode 100644
> tests/tcg/hexagon/unaligned_pc_multi_cof.S
>
> a/tests/tcg/hexagon/unaligned_pc.c b/tests/tcg/hexagon/unaligned_pc.c
> new file mode 100644
> index 00..1add2d0d99
> --- /dev/null
> +++ b/tests/tcg/hexagon/unaligned_pc.c
> @@ -0,0 +1,85 @@
> +#include
> +#include
> +#include
> +#include
> +
> +/* will be changed in signal handler */ volatile sig_atomic_t
> +completed_tests; static jmp_buf after_test; static int nr_tests;
> +
> +void __attribute__((naked)) test_return(void) {
> +asm volatile(
> +"allocframe(#0x8)\n"
> +"r0 = #0x\n"
> +"framekey = r0\n"
> +"dealloc_return\n"
> +: : : "r0");
Add r29, r30, r31 to clobbers list.
Add framekey to clobbers list (assuming the compiler will take it).
> +}
> +
> +void test_endloop(void)
> +{
> +asm volatile(
> +"loop0(1f, #2)\n"
> +"1: r0 = #0x3\n"
> +"sa0 = r0\n"
> +"{ nop }:endloop0\n"
> +: : : "r0");
> +}
Add sa0, lc0, usr to the clobbers list.
> +
> +void test_multi_cof(void)
> +{
> +asm volatile(
> +"p0 = cmp.eq(r0, r0)\n"
> +"{\n"
> +"if (p0) jump test_multi_cof_unaligned\n"
> +"jump 1f\n"
> +"}\n"
> +"1: nop\n"
> +: : : "p0");
> +}
> +
> +void sigbus_handler(int signum)
> +{
> +/* retore framekey after test_return */
> +asm volatile(
> +"r0 = #0\n"
> +"framekey = r0\n"
> +: : : "r0");
Add framekey to the clobbers list.
> +printf("Test %d complete\n", completed_tests);
> +completed_tests++;
> +siglongjmp(after_test, 1);
> +}
> +
> +void test_done(void)
> +{
> +int err = (completed_tests != nr_tests);
> +puts(err ? "FAIL" : "PASS");
> +exit(err);
> +}
> +
> +typedef void (*test_fn)(void);
> +
> +int main()
> +{
> +test_fn tests[] = { test_return, test_endloop, test_multi_cof,
test_done
> };
> +nr_tests = (sizeof(tests) / sizeof(tests[0])) - 1;
> +
> +struct sigaction sa = {
> +.sa_sigaction = sigbus_handler,
> +.sa_flags = SA_SIGINFO
> +};
> +
> +if (sigaction(SIGBUS, &sa, NULL) < 0) {
> +perror("sigaction");
> +return EXIT_FAILURE;
> +}
> +
> +sigsetjmp(after_test, 1);
> +tests[completed_tests]();
> +
> +/* should never get here */
> +puts("FAIL");
> +return 1;
> +}
> diff --git a/tests/tcg/hexagon/Makefile.target
> b/tests/tcg/hexagon/Makefile.target
> index f839b2c0d5..75139e731c 100644
> --- a/tests/tcg/hexagon/Makefile.target
> +++ b/tests/tcg/hexagon/Makefile.target
> @@ -51,6 +51,7 @@ HEX_TESTS += scatter_gather HEX_TESTS += hvx_misc
> HEX_TESTS += hvx_histogram HEX_TESTS += invalid-slots
> +HEX_TESTS += unaligned_pc
>
> run-and-check-exception = $(call run-test,$2,$3 2>$2.stderr; \
> test $$? -eq 1 && grep -q "exception $(strip $1)" $2.stderr) @@ -
> 108,6 +109,9 @@ preg_alias: preg_alias.c hex_test.h
> read_write_overlap: read_write_overlap.c hex_test.h
> reg_mut: reg_mut.c hex_test.h
>
> +unaligned_pc: unaligned_pc.c unaligned_pc_multi_cof.S
> + $(CC) $(CFLAGS) $(CROSS_CC_GUEST_CFLAGS) -mv73 $^ -o $@
> $(LDFLAGS)
> +
> # This test has to be compiled for the -mv67t target
> usr: usr.c hex_test.h
> $(CC) $(CFLAGS) -mv67t -O2 -Wno-inline-asm -Wno-expansion-to-
> defined $< -o $@ $(LDFLAGS) diff --git
> a/tests/tcg/hexagon/unaligned_pc_multi
RE: [PATCH] Hexagon: add PC alignment check and exception
PS You should also update the pkt_raises_exception function in translate.c to return true for packets that contain these instructions. This will ensure that none of the machine state is changed before the check is complete. Taylor > -Original Message- > From: ltaylorsimp...@gmail.com > Sent: Monday, April 29, 2024 9:41 AM > To: 'Matheus Tavares Bernardino' ; qemu- > de...@nongnu.org > Cc: bc...@quicinc.com; sidn...@quicinc.com; a...@rev.ng; a...@rev.ng > Subject: RE: [PATCH] Hexagon: add PC alignment check and exception > > > > > -Original Message- > > From: Matheus Tavares Bernardino > > Sent: Friday, April 26, 2024 1:16 PM > > To: qemu-devel@nongnu.org > > Cc: bc...@quicinc.com; sidn...@quicinc.com; a...@rev.ng; a...@rev.ng; > > ltaylorsimp...@gmail.com > > Subject: [PATCH] Hexagon: add PC alignment check and exception > > > > The Hexagon Programmer's Reference Manual says that the exception > 0x1e > > should be raised upon an unaligned program counter. Let's implement > > that and also add tests for both the most common case as well as > > packets with multiple change-of-flow instructions. > > > > Signed-off-by: Matheus Tavares Bernardino > > > --- > > > > --- a/target/hexagon/genptr.c > > +++ b/target/hexagon/genptr.c > > @@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext > > You haven't added the check to gen_write_new_pc_pcrel. It's not needed > there because the encoding guarantees the target is always aligned - right? > However, there is a call to gen_write_new_pc_addr inside that function. In > this case, we'll add a check that isn't necessary. Consider adding a parameter > to indicate if the check can be avoided. > > > > a/tests/tcg/hexagon/Makefile.target > > b/tests/tcg/hexagon/Makefile.target > > index f839b2c0d5..02d7fff34c 100644 > > --- a/tests/tcg/hexagon/Makefile.target > > +++ b/tests/tcg/hexagon/Makefile.target > > @@ -51,6 +51,19 @@ HEX_TESTS += scatter_gather HEX_TESTS += > hvx_misc > > HEX_TESTS += hvx_histogram HEX_TESTS += invalid-slots > > +HEX_TESTS += unaligned_pc > > +HEX_TESTS += unaligned_pc_multi_cof > > + > > +run-unaligned_pc: unaligned_pc > > +run-unaligned_pc_multi_cof: unaligned_pc_multi_cof run-unaligned_pc > > +run-unaligned_pc_multi_cof: > > + $(call run-test, $<, $(QEMU) $< 2> $<.stderr,"$< on > > $(TARGET_NAME)"); \ > > + if [ $$? -ne 1 ] ; then \ > > + return 1; \ > > + fi > > + $(call quiet-command, \ > > + grep -q "exception 0x1e" $<.stderr, \ > > + "GREP", "exception 0x1e"); > > We should also test endloop instructions. > > Thanks, > Taylor >
RE: [PATCH] Hexagon: add PC alignment check and exception
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Friday, April 26, 2024 1:16 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; sidn...@quicinc.com; a...@rev.ng; a...@rev.ng; > ltaylorsimp...@gmail.com > Subject: [PATCH] Hexagon: add PC alignment check and exception > > The Hexagon Programmer's Reference Manual says that the exception 0x1e > should be raised upon an unaligned program counter. Let's implement that > and also add tests for both the most common case as well as packets with > multiple change-of-flow instructions. > > Signed-off-by: Matheus Tavares Bernardino > --- > --- a/target/hexagon/genptr.c > +++ b/target/hexagon/genptr.c > @@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext You haven't added the check to gen_write_new_pc_pcrel. It's not needed there because the encoding guarantees the target is always aligned - right? However, there is a call to gen_write_new_pc_addr inside that function. In this case, we'll add a check that isn't necessary. Consider adding a parameter to indicate if the check can be avoided. > a/tests/tcg/hexagon/Makefile.target b/tests/tcg/hexagon/Makefile.target > index f839b2c0d5..02d7fff34c 100644 > --- a/tests/tcg/hexagon/Makefile.target > +++ b/tests/tcg/hexagon/Makefile.target > @@ -51,6 +51,19 @@ HEX_TESTS += scatter_gather HEX_TESTS += hvx_misc > HEX_TESTS += hvx_histogram HEX_TESTS += invalid-slots > +HEX_TESTS += unaligned_pc > +HEX_TESTS += unaligned_pc_multi_cof > + > +run-unaligned_pc: unaligned_pc > +run-unaligned_pc_multi_cof: unaligned_pc_multi_cof run-unaligned_pc > +run-unaligned_pc_multi_cof: > + $(call run-test, $<, $(QEMU) $< 2> $<.stderr,"$< on > $(TARGET_NAME)"); \ > + if [ $$? -ne 1 ] ; then \ > + return 1; \ > + fi > + $(call quiet-command, \ > + grep -q "exception 0x1e" $<.stderr, \ > + "GREP", "exception 0x1e"); We should also test endloop instructions. Thanks, Taylor
RE: [PATCH 4/9] Hexagon (target/hexagon) Mark has_pred_dest in trans functions
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Tuesday, February 27, 2024 8:21 AM > To: Taylor Simpson > Cc: qemu-devel@nongnu.org; bc...@quicinc.com; > quic_mathb...@quicinc.com; sidn...@quicinc.com; > quic_mlie...@quicinc.com; richard.hender...@linaro.org; > phi...@linaro.org; a...@rev.ng; a...@rev.ng > Subject: Re: [PATCH 4/9] Hexagon (target/hexagon) Mark has_pred_dest in > trans functions > > On Mon, 26 Feb 2024 13:17:17 -0700 Taylor Simpson > wrote: > > > > diff --git a/target/hexagon/gen_trans_funcs.py > > b/target/hexagon/gen_trans_funcs.py > > index 07292e0170..f1972fd2dd 100755 > > --- a/target/hexagon/gen_trans_funcs.py > > +++ b/target/hexagon/gen_trans_funcs.py > > @@ -86,6 +86,7 @@ def gen_trans_funcs(f): > > > > new_read_idx = -1 > > dest_idx = -1 > > +has_pred_dest = "false" > > for regno, regstruct in enumerate(regs): > > reg_type, reg_id, _, _ = regstruct > > reg = hex_common.get_register(tag, reg_type, reg_id) @@ > > -96,6 +97,8 @@ def gen_trans_funcs(f): > > new_read_idx = regno > > if reg.is_written() and dest_idx == -1: > > dest_idx = regno > > +if reg_type == "P" and not reg.is_read(): > > +has_pred_dest = "true" > > I got a bit confused here. Why do we use "not reg.is_read()"? I though this > would be "reg.is_written()". The original C code is if ((strstr(opcode_wregs[opcode], "Pd4") || strstr(opcode_wregs[opcode], "Pe4")) && Checking for reg.is_written() would also match Px4, which is read-write. Would it be more clear if we checked for reg.is_written() and non reg.is_read()? Thanks, Taylor
RE: [PATCH 2/9] Hexagon (target/hexagon) Mark new_read_idx in trans functions
> -Original Message-
> From: Matheus Tavares Bernardino
> Sent: Tuesday, February 27, 2024 8:20 AM
> To: Taylor Simpson
> Cc: qemu-devel@nongnu.org; bc...@quicinc.com;
> quic_mathb...@quicinc.com; sidn...@quicinc.com;
> quic_mlie...@quicinc.com; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: Re: [PATCH 2/9] Hexagon (target/hexagon) Mark new_read_idx in
> trans functions
>
> On Mon, 26 Feb 2024 13:17:15 -0700 Taylor Simpson
> wrote:
> >
> > diff --git a/target/hexagon/gen_trans_funcs.py
> > b/target/hexagon/gen_trans_funcs.py
> > index 53e844a44b..79475b2946 100755
> > --- a/target/hexagon/gen_trans_funcs.py
> > +++ b/target/hexagon/gen_trans_funcs.py
> > @@ -84,14 +84,15 @@ def gen_trans_funcs(f):
> > insn->opcode = {tag};
> > """))
> >
> > -regno = 0
> > -for reg in regs:
> > -reg_type = reg[0]
> > -reg_id = reg[1]
> > +new_read_idx = -1
> > +for regno, regstruct in enumerate(regs):
> > +reg_type, reg_id, _, _ = regstruct
> > +reg = hex_common.get_register(tag, reg_type, reg_id)
>
> Nit: since we don't care about the remaining elements of regstruct, we
could
> simplify (and future-proof) this even further to:
>
> reg_type, reg_id, *_ = regstruct
>
> Or perhaps even eliminate the variable entirely:
>
> for regno, (reg_type, reg_id, *_) in enumerate(regs):
> ...
Let's go with the second option. I'll make the change.
Thanks,
Taylor
RE: [PATCH v2 1/3] Hexagon (target/hexagon) Use QEMU decodetree (32-bit instructions)
> -Original Message-
> From: Brian Cain
> Sent: Sunday, January 14, 2024 5:21 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) ; Sid
> Manning ; Marco Liebel (QUIC)
> ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [PATCH v2 1/3] Hexagon (target/hexagon) Use QEMU
> decodetree (32-bit instructions)
>
>
>
> > -Original Message-
> > From: Taylor Simpson
> > Sent: Monday, January 8, 2024 4:49 PM
> > To: qemu-devel@nongnu.org
> > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > ; Sid Manning ;
> Marco
> > Liebel (QUIC) ;
> > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > a...@rev.ng; ltaylorsimp...@gmail.com
> > Subject: [PATCH v2 1/3] Hexagon (target/hexagon) Use QEMU decodetree
> > (32- bit instructions)
> >
> >
> > The Decodetree Specification can be found here
> > https://www.qemu.org/docs/master/devel/decodetree.html
> >
> > Covers all 32-bit instructions, including HVX
> >
> > We generate separate decoders for each instruction class. The reason
> > will be more apparent in the next patch in this series.
> >
> > We add 2 new scripts
> > gen_decodetree.pyGenerate the input to decodetree.py
> > gen_trans_funcs.py Generate the trans_* functions used by the
> > output of decodetree.py
> >
> > Since the functions generated by decodetree.py take DisasContext * as
> > an argument, we add the argument to a couple of functions that didn't
> > need it previously. We also set the insn field in DisasContext during
> > decode because it is used by the trans_* functions.
> >
> > There is a g_assert_not_reached() in decode_insns() in decode.c to
> > verify we never try to use the old decoder on 32-bit instructions
> >
> > Signed-off-by: Taylor Simpson
> > ---
> > target/hexagon/decode.h | 5 +-
> > target/hexagon/decode.c | 54 -
> > target/hexagon/translate.c| 4 +-
> > target/hexagon/README | 13 +-
> > target/hexagon/gen_decodetree.py | 193
> > ++
> target/hexagon/gen_trans_funcs.py | 132
> > target/hexagon/meson.build| 55 +
> > 7 files changed, 442 insertions(+), 14 deletions(-) create mode
> > 100755 target/hexagon/gen_decodetree.py create mode 100755
> > target/hexagon/gen_trans_funcs.py
> >
>
> LGTM, but some nitpicky suggestions:
>
> diff --git a/target/hexagon/gen_decodetree.py
> b/target/hexagon/gen_decodetree.py
> index 2dff975f55..62bd8a62b6 100755
> --- a/target/hexagon/gen_decodetree.py
> +++ b/target/hexagon/gen_decodetree.py
> @@ -57,7 +57,7 @@ def ordered_unique(l):
> "d",
> "e",
> "f",
> -"g"
> +"g",
> }
>
> #
> @@ -104,9 +104,6 @@ def gen_decodetree_file(f, class_to_decode):
> if skip_tag(tag, class_to_decode):
> continue
>
> -f.write("")
> -f.write("\n")
> -
> enc = encs[tag]
> enc_str = "".join(reversed(encs[tag]))
>
> @@ -115,21 +112,21 @@ def gen_decodetree_file(f, class_to_decode):
> if is_subinsn:
> enc_str = "---" + enc_str
>
> -f.write(f"## {tag}:\t{enc_str}\n")
> -f.write("##\n")
> +f.write(("#" * 80) + "\n"
> +f"## {tag}:\t{enc_str}\n"
> +"##\n")
>
> regs = ordered_unique(regre.findall(iset.iset[tag]["syntax"]))
> imms = ordered_unique(immre.findall(iset.iset[tag]["syntax"]))
>
> # Write the field definitions for the registers
> -regno = 0
> -for reg in regs:
> -reg_type = reg[0]
> -reg_id = reg[1]
> +for regno, reg in enumerate(regs):
> +reg_type, reg_id, _, reg_enc_size = reg
> reg_letter = reg_id[0]
> -reg_num_choices = int(reg[3].rstrip("S"))
> -reg_mapping = reg[0] + "".join(["_" for letter in reg[1]]) +
> reg[3]
> +reg_num_choices = int(reg_enc_size.rstrip("S"))
> +reg_mapping = reg_type + "".join("_" for letter in reg_id)
> + + reg_enc_size
> reg_enc_fields = re.findall(reg_letter + "+", enc)
> +print(f'{reg} -> {reg_enc_fields}')
>
> # Check for some errors
> if len(reg_enc_fields) == 0:
> @@ -140,13 +137,12 @@ def gen_decodetree_file(f, class_to_decode):
> if 2 ** len(reg_enc_field) != reg_num_choices:
> raise Exception(f"{tag} has incorrect register field width!")
>
> -f.write(f"%{tag}_{reg_type}{reg_id}\t")
> -f.write(f"{enc.index(reg_enc_field)}:{len(reg_enc_field)}")
> +f.write(f"%{tag}_{reg_type}{reg_id}\t"
> +f"{enc.index(reg_enc_field)}:{len(reg_enc_field)}")
> if (reg_type in num_registers and
> reg_num_choices != num_registers[re
RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object oriented - gen_helper_protos
> -Original Message-
> From: ltaylorsimp...@gmail.com
> Sent: Tuesday, December 5, 2023 11:08 AM
> To: 'Brian Cain' ; qemu-devel@nongnu.org
> Cc: 'Matheus Bernardino (QUIC)' ; 'Sid
> Manning' ; 'Marco Liebel (QUIC)'
> ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object
> oriented - gen_helper_protos
>
>
>
> > -Original Message-
> > From: Brian Cain
> > Sent: Monday, December 4, 2023 9:46 PM
> > To: Taylor Simpson ; qemu-
> de...@nongnu.org
> > Cc: Matheus Bernardino (QUIC) ; Sid
> Manning
> > ; Marco Liebel (QUIC)
> ;
> > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > a...@rev.ng
> > Subject: RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators
> > object oriented - gen_helper_protos
> >
> >
> >
> > > -Original Message-
> > > From: Taylor Simpson
> > > Sent: Monday, December 4, 2023 7:53 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > > ; Sid Manning ;
> > Marco
> > > Liebel (QUIC) ;
> > > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > > a...@rev.ng; ltaylorsimp...@gmail.com
> > > Subject: [PATCH 3/9] Hexagon (target/hexagon) Make generators object
> > > oriented - gen_helper_protos
> > >
> > > Signed-off-by: Taylor Simpson
> > > ---
> > > target/hexagon/gen_helper_protos.py | 184
> > > target/hexagon/hex_common.py| 15 +--
> > > 2 files changed, 55 insertions(+), 144 deletions(-)
> > >
> > > diff --git a/target/hexagon/gen_helper_protos.py
> > > b/target/hexagon/gen_helper_protos.py
> > > index 131043795a..9277199e1d 100755
> > > --- a/target/hexagon/gen_helper_protos.py
> > > +++ b/target/hexagon/gen_helper_protos.py
> > > ##
> > > ## Generate the DEF_HELPER prototype for an instruction
> > > ## For A2_add: Rd32=add(Rs32,Rt32)
> > > @@ -65,116 +32,62 @@ def gen_helper_prototype(f, tag, tagregs,
> > tagimms):
> > > regs = tagregs[tag]
> > > imms = tagimms[tag]
> > >
> > > -numresults = 0
> > > +## If there is a scalar result, it is the return type
> > > +return_type = ""
> >
> > Should we use `return_type = None` here?
> >
> > > numscalarresults = 0
> > > -numscalarreadwrite = 0
> > > for regtype, regid in regs:
> > > -if hex_common.is_written(regid):
> > > -numresults += 1
> > > -if hex_common.is_scalar_reg(regtype):
> > > +reg = hex_common.get_register(tag, regtype, regid)
> > > +if reg.is_written() and reg.is_scalar_reg():
> > > +return_type = reg.helper_proto_type()
> > > numscalarresults += 1
> > > -if hex_common.is_readwrite(regid):
> > > -if hex_common.is_scalar_reg(regtype):
> > > -numscalarreadwrite += 1
> > > +if numscalarresults == 0:
> > > +return_type = "void"
> >
> > Should we use `return_type = None` here?
>
> I don't see a point of setting it to None. By the time it gets to the use
> below,
> it will definitely have a value. We could initialize it to void instead of
> "" and
> skip this check.
>
>
> > > +
> > > +## Other stuff the helper might need
> > > +if hex_common.need_pkt_has_multi_cof(tag):
> > > +declared.append("i32")
> > > +if hex_common.need_pkt_need_commit(tag):
> > > +declared.append("i32")
> > > +if hex_common.need_PC(tag):
> > > +declared.append("i32")
> > > +if hex_common.need_next_PC(tag):
> > > +declared.append("i32")
> > > +if hex_common.need_slot(tag):
> > > +declared.append("i32")
> > > +if hex_common.need_part1(tag):
> > > +declared.append("i32")
> >
> > What do you think of this instead? The delta below is on top of this patch.
> >
> > --- a/target/hexagon/gen_helper_protos.py
> > +++ b/target/hexagon/gen_helper_protos.py
> > @@ -73,18 +73,9 @@ def gen_helper_prototype(f, tag, tagregs, tagimms):
> > declared.append("s32")
> >
> > ## Other stuff the helper might need
> > -if hex_common.need_pkt_has_multi_cof(tag):
> > -declared.append("i32")
> > -if hex_common.need_pkt_need_commit(tag):
> > -declared.append("i32")
> > -if hex_common.need_PC(tag):
> > -declared.append("i32")
> > -if hex_common.need_next_PC(tag):
> > -declared.append("i32")
> > -if hex_common.need_slot(tag):
> > -declared.append("i32")
> > -if hex_common.need_part1(tag):
> > -declared.append("i32")
> > +for stuff in hex_common.other_stuff:
> > +if stuff(tag):
> > +declared.append('i32')
> >
> > arguments = ", ".join(declared)
> > f.write(f"DEF_HELPER_{len(declared) - 1}({tag}, {arguments})\n")
> > diff --git a/target/hexagon/gen_tcg_funcs.py
> > b/target/hexagon/gen_tcg_funcs.py index 8c2bc03c10..cb02d91886 100755
> > --- a/target/hexagon/gen_tcg_funcs.py
> > +++ b/target/hexagon/ge
RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object oriented - gen_helper_protos
> -Original Message-
> From: Brian Cain
> Sent: Monday, December 4, 2023 9:46 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) ; Sid
> Manning ; Marco Liebel (QUIC)
> ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object
> oriented - gen_helper_protos
>
>
>
> > -Original Message-
> > From: Taylor Simpson
> > Sent: Monday, December 4, 2023 7:53 PM
> > To: qemu-devel@nongnu.org
> > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > ; Sid Manning ;
> Marco
> > Liebel (QUIC) ;
> > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > a...@rev.ng; ltaylorsimp...@gmail.com
> > Subject: [PATCH 3/9] Hexagon (target/hexagon) Make generators object
> > oriented - gen_helper_protos
> >
> > Signed-off-by: Taylor Simpson
> > ---
> > target/hexagon/gen_helper_protos.py | 184
> > target/hexagon/hex_common.py| 15 +--
> > 2 files changed, 55 insertions(+), 144 deletions(-)
> >
> > diff --git a/target/hexagon/gen_helper_protos.py
> > b/target/hexagon/gen_helper_protos.py
> > index 131043795a..9277199e1d 100755
> > --- a/target/hexagon/gen_helper_protos.py
> > +++ b/target/hexagon/gen_helper_protos.py
> > ##
> > ## Generate the DEF_HELPER prototype for an instruction
> > ## For A2_add: Rd32=add(Rs32,Rt32)
> > @@ -65,116 +32,62 @@ def gen_helper_prototype(f, tag, tagregs,
> tagimms):
> > regs = tagregs[tag]
> > imms = tagimms[tag]
> >
> > -numresults = 0
> > +## If there is a scalar result, it is the return type
> > +return_type = ""
>
> Should we use `return_type = None` here?
>
> > numscalarresults = 0
> > -numscalarreadwrite = 0
> > for regtype, regid in regs:
> > -if hex_common.is_written(regid):
> > -numresults += 1
> > -if hex_common.is_scalar_reg(regtype):
> > +reg = hex_common.get_register(tag, regtype, regid)
> > +if reg.is_written() and reg.is_scalar_reg():
> > +return_type = reg.helper_proto_type()
> > numscalarresults += 1
> > -if hex_common.is_readwrite(regid):
> > -if hex_common.is_scalar_reg(regtype):
> > -numscalarreadwrite += 1
> > +if numscalarresults == 0:
> > +return_type = "void"
>
> Should we use `return_type = None` here?
I don't see a point of setting it to None. By the time it gets to the use
below, it will definitely have a value. We could initialize it to void instead
of "" and skip this check.
> > +
> > +## Other stuff the helper might need
> > +if hex_common.need_pkt_has_multi_cof(tag):
> > +declared.append("i32")
> > +if hex_common.need_pkt_need_commit(tag):
> > +declared.append("i32")
> > +if hex_common.need_PC(tag):
> > +declared.append("i32")
> > +if hex_common.need_next_PC(tag):
> > +declared.append("i32")
> > +if hex_common.need_slot(tag):
> > +declared.append("i32")
> > +if hex_common.need_part1(tag):
> > +declared.append("i32")
>
> What do you think of this instead? The delta below is on top of this patch.
>
> --- a/target/hexagon/gen_helper_protos.py
> +++ b/target/hexagon/gen_helper_protos.py
> @@ -73,18 +73,9 @@ def gen_helper_prototype(f, tag, tagregs, tagimms):
> declared.append("s32")
>
> ## Other stuff the helper might need
> -if hex_common.need_pkt_has_multi_cof(tag):
> -declared.append("i32")
> -if hex_common.need_pkt_need_commit(tag):
> -declared.append("i32")
> -if hex_common.need_PC(tag):
> -declared.append("i32")
> -if hex_common.need_next_PC(tag):
> -declared.append("i32")
> -if hex_common.need_slot(tag):
> -declared.append("i32")
> -if hex_common.need_part1(tag):
> -declared.append("i32")
> +for stuff in hex_common.other_stuff:
> +if stuff(tag):
> +declared.append('i32')
>
> arguments = ", ".join(declared)
> f.write(f"DEF_HELPER_{len(declared) - 1}({tag}, {arguments})\n") diff
> --git
> a/target/hexagon/gen_tcg_funcs.py b/target/hexagon/gen_tcg_funcs.py
> index 8c2bc03c10..cb02d91886 100755
> --- a/target/hexagon/gen_tcg_funcs.py
> +++ b/target/hexagon/gen_tcg_funcs.py
> @@ -109,18 +109,9 @@ def gen_tcg_func(f, tag, regs, imms):
>
> declared.append(f"tcg_constant_tl({hex_common.imm_name(immlett)})")
>
> ## Other stuff the helper might need
> -if hex_common.need_pkt_has_multi_cof(tag):
> -declared.append("tcg_constant_tl(ctx->pkt->pkt_has_multi_cof)")
> -if hex_common.need_pkt_need_commit(tag):
> -declared.append("tcg_constant_tl(ctx->need_commit)")
> -if hex_common.need_PC(tag):
> -declared.append("tcg_constant_tl(ctx->pkt->pc)")
> -if hex_common.need_next_PC(tag):
> -declared.append("tcg_constant_tl(ctx->nex
RE: [RFC PATCH] Hexagon (target/hexagon) Make generators object oriented
> -Original Message-
> From: Brian Cain
> Sent: Thursday, November 16, 2023 10:25 AM
> To: ltaylorsimp...@gmail.com; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) ; Sid
> Manning ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators object
> oriented
>
>
>
> > -Original Message-
> > From: ltaylorsimp...@gmail.com
> > Sent: Wednesday, November 15, 2023 4:03 PM
> > To: Brian Cain ; qemu-devel@nongnu.org
> > Cc: Matheus Bernardino (QUIC) ; Sid
> Manning
> > ; richard.hender...@linaro.org;
> > phi...@linaro.org; a...@rev.ng; a...@rev.ng
> > Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators
> > object oriented
> >
> > > -Original Message-
> > > From: Brian Cain
> > > Sent: Wednesday, November 15, 2023 1:51 PM
> > > To: Taylor Simpson ; qemu-
> de...@nongnu.org
> > > Cc: Matheus Bernardino (QUIC) ; Sid
> > > Manning ; richard.hender...@linaro.org;
> > > phi...@linaro.org; a...@rev.ng; a...@rev.ng
> > > Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators
> > > object oriented
> > >
> > >
> > >
> > > > -Original Message-
> > > > From: Taylor Simpson
> > > > Sent: Thursday, November 9, 2023 3:26 PM
> > > > To: qemu-devel@nongnu.org
> > > > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > > > ; Sid Manning ;
> > > > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > > a...@rev.ng;
> > > > ltaylorsimp...@gmail.com
> > > > Subject: [RFC PATCH] Hexagon (target/hexagon) Make generators
> > > > object oriented
> > > >
> > > > RFC - This patch handles gen_tcg_funcs.py. I'd like to get
> > > > comments on the general approach before working on the other
> Python scripts.
> > > >
> > > > The generators are generally a bunch of Python if-then-else
> > > > statements based on the regtype and regid. Encapsulate
> > > > regtype/regid into a class hierarchy. Clients lookup the register
> > > > and invoke methods.
> > > >
> > > > This has several advantages for making the code easier to read,
> > > > understand, and maintain
> > > > - The class name makes it more clear what the operand does
> > > > - All the methods for a given type of operand are together
> > > > - Don't need as many calls to hex_common.bad_register
> > > > - We can remove the functions in hex_common that use regtype/regid
> > > > (e.g., is_read)
> > > >
> > > > Signed-off-by: Taylor Simpson
> > > > ---
> > > > diff --git a/target/hexagon/hex_common.py
> > > b/target/hexagon/hex_common.py
> > > > index 0da65d6dd6..13ee55b6b2 100755
> > > > --- a/target/hexagon/hex_common.py
> > > > +++ b/target/hexagon/hex_common.py
> > > > +class PredReadWrite(ReadWrite):
> > > > +def genptr_decl(self, f, tag, regno):
> > > > +f.write(f"const int {self.regN} = insn->regno[{regno}];\n")
> > > > +f.write(f"TCGv {self.regV} = tcg_temp_new();\n")
> > > > +f.write(f"tcg_gen_mov_tl({self.regV},
> hex_pred[{self.regN}]);\n")
> > >
> > > Instead of successive calls to f.write(), each passing their own
> > > string with a newline, use triple quotes:
> > >
> > > f.write(f"""const int {self.regN} = insn->regno[{regno}];
> > > TCGv {self.regV} = tcg_temp_new();
> > > tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);\n""")
> > >
> > > If necessary/appropriate, you can also use textwrap.dedent() to make
> > > the leading whitespace look appropriate:
> > > https://docs.python.org/3/library/textwrap.html#textwrap.dedent
> > >
> > > import textwrap
> > > ...
> > > f.write(textwrap.dedent(f"""const int {self.regN} = insn-
> >regno[{regno}];
> > > TCGv {self.regV} = tcg_temp_new();
> > > tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);\n"""))
> > >
> >
> > The indenting is for the readability of the output. We could dedent
> > everything and run the result through the indent utility as
> > idef-parser does. Not sure it's worth it though.
>
> Regardless of textwrap.dedent(), I think the output is most readable with
> triple-quotes. It's more readable than it is with multiple f.write("this
> line\n")
> invocations.
>
> The intent of calling textwrap.dedent is to allow you to not out-dent the text
> in the python program. Since the indentation of the other lines next to the
> string literal are significant to the program, it might be odd/confusing to
> see
> the python program have out-dented text lines.
>
> Consider the readability of the python program:
>
> if foo:
> f.write(textwrap.dedent(f"""\
> const int {self.regN} = insn->regno[{regno}];
> TCGv {self.regV} = tcg_temp_new();
> tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);
> """))
> if bar:
> f.write(textwrap.dedent(f"""\
> int x = {bar.reg};
> """)
> vs
>
> if foo:
> f.write(f"""\
> const int {self.regN} = insn->regno[{regno}]; TCGv {self.regV} =
> tcg_temp_new(); tcg_gen_mov_tl({self.reg
RE: [RFC PATCH] Hexagon (target/hexagon) Make generators object oriented
> -Original Message-
> From: Brian Cain
> Sent: Wednesday, November 15, 2023 1:51 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) ; Sid
> Manning ; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators object
> oriented
>
>
>
> > -Original Message-
> > From: Taylor Simpson
> > Sent: Thursday, November 9, 2023 3:26 PM
> > To: qemu-devel@nongnu.org
> > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > ; Sid Manning ;
> > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> a...@rev.ng;
> > ltaylorsimp...@gmail.com
> > Subject: [RFC PATCH] Hexagon (target/hexagon) Make generators object
> > oriented
> >
> > RFC - This patch handles gen_tcg_funcs.py. I'd like to get comments
> > on the general approach before working on the other Python scripts.
> >
> > The generators are generally a bunch of Python if-then-else
> > statements based on the regtype and regid. Encapsulate regtype/regid
> > into a class hierarchy. Clients lookup the register and invoke
> > methods.
> >
> > This has several advantages for making the code easier to read,
> > understand, and maintain
> > - The class name makes it more clear what the operand does
> > - All the methods for a given type of operand are together
> > - Don't need as many calls to hex_common.bad_register
> > - We can remove the functions in hex_common that use regtype/regid
> > (e.g., is_read)
> >
> > Signed-off-by: Taylor Simpson
> > ---
> > diff --git a/target/hexagon/hex_common.py
> b/target/hexagon/hex_common.py
> > index 0da65d6dd6..13ee55b6b2 100755
> > --- a/target/hexagon/hex_common.py
> > +++ b/target/hexagon/hex_common.py
> > +class ModifierSource(Source):
> > +def genptr_decl(self, f, tag, regno):
> > +f.write(f"const int {self.regN} = insn->regno[{regno}];\n")
> > +f.write(f"TCGv {self.regV} = hex_gpr[{self.regN} +
> HEX_REG_M0];\n")
> > +def idef_arg(self, declared):
> > +declared.append(self.regV)
> > +declared.append(self.regN)
> > +
>
> IMO it's easier to reason about a function if it doesn't modify its inputs and
> instead it returns the transformed input. If idef_arg instead returned a new
> list or returned an iterable for the caller to catenate, it would be clearer.
We should figure out a better way to handle the special case of modifier
registers. For every other register type,
Idef_arg simply returns self.regV. For circular addressing, we also need the
value of the corresponding CS register. Currently,
we solve this by passing the register number so that idef-parser can get the
value (i.e., hex_gpr[HEX_REG_CS0 + self.regN]).
We could have idef-parser skip the circular addressing instructions (it already
skips the bit-reverse instructions). That seems
like a big hammer though. Any other thoughts?
> > +class PredReadWrite(ReadWrite):
> > +def genptr_decl(self, f, tag, regno):
> > +f.write(f"const int {self.regN} = insn->regno[{regno}];\n")
> > +f.write(f"TCGv {self.regV} = tcg_temp_new();\n")
> > +f.write(f"tcg_gen_mov_tl({self.regV},
> > hex_pred[{self.regN}]);\n")
>
> Instead of successive calls to f.write(), each passing their own string with a
> newline, use triple quotes:
>
> f.write(f"""const int {self.regN} = insn->regno[{regno}];
> TCGv {self.regV} = tcg_temp_new();
> tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);\n""")
>
> If necessary/appropriate, you can also use textwrap.dedent() to make the
> leading whitespace look appropriate:
> https://docs.python.org/3/library/textwrap.html#textwrap.dedent
>
> import textwrap
> ...
> f.write(textwrap.dedent(f"""const int {self.regN} = insn->regno[{regno}];
> TCGv {self.regV} = tcg_temp_new();
> tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);\n"""))
>
The indenting is for the readability of the output. We could dedent everything
and run the result through the indent utility
as idef-parser does. Not sure it's worth it though.
> > +def init_registers():
> > +registers["Rd"] = GprDest("R", "d")
> > +registers["Re"] = GprDest("R", "e")
> > +registers["Rs"] = GprSource("R", "s")
> > +registers["Rt"] = GprSource("R", "t")
> > +registers["Ru"] = GprSource("R", "u")
> > +registers["Rv"] = GprSource("R", "v")
> > +registers["Rx"] = GprReadWrite("R", "x")
> > +registers["Ry"] = GprReadWrite("R", "y")
> > +registers["Cd"] = ControlDest("C", "d")
> > +registers["Cs"] = ControlSource("C", "s")
> > +registers["Mu"] = ModifierSource("M", "u")
> > +registers["Pd"] = PredDest("P", "d")
> > +registers["Pe"] = PredDest("P", "e")
> > +registers["Ps"] = PredSource("P", "s")
> > +registers["Pt"] = PredSource("P", "t")
> > +registers["Pu"] = PredSource("P", "u")
> > +registers["Pv"] = PredSource("P", "v")
> > +registers["Px"] = PredReadWrite("P", "x")
>
RE: [PATCH 0/3] Hexagon (target/hexagon) Enable more short-circuit packets
Looks like a problem with git send-email ☹ I will troubleshoot ... > -Original Message- > From: Anton Johansson > Sent: Thursday, November 2, 2023 6:44 PM > To: Taylor Simpson > Cc: qemu-devel@nongnu.org > Subject: Re: [PATCH 0/3] Hexagon (target/hexagon) Enable more short- > circuit packets > > Hi, Taylor!:) Always nice to see your name pop up here. The patches seem to > have been sent as attachments for whatever reason. > > / Anton
RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> -Original Message- > From: Brian Cain > Sent: Sunday, October 8, 2023 7:50 AM > To: ltaylorsimp...@gmail.com; qemu-devel@nongnu.org > Cc: arm...@redhat.com; richard.hender...@linaro.org; phi...@linaro.org; > peter.mayd...@linaro.org; Matheus Bernardino (QUIC) > ; stefa...@redhat.com; a...@rev.ng; > a...@rev.ng; Marco Liebel (QUIC) > Subject: RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > > > > -Original Message- > > From: ltaylorsimp...@gmail.com > > Sent: Friday, October 6, 2023 11:01 AM > > To: Brian Cain ; qemu-devel@nongnu.org > > Cc: arm...@redhat.com; richard.hender...@linaro.org; > > phi...@linaro.org; peter.mayd...@linaro.org; Matheus Bernardino (QUIC) > > ; stefa...@redhat.com; a...@rev.ng; > > a...@rev.ng; Marco Liebel (QUIC) > > Subject: RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > > > WARNING: This email originated from outside of Qualcomm. Please be > > wary of any links or attachments, and do not enable macros. > > > > > -Original Message- > > > From: Brian Cain > > > Sent: Thursday, October 5, 2023 4:22 PM > > > To: qemu-devel@nongnu.org > > > Cc: bc...@quicinc.com; arm...@redhat.com; > > > richard.hender...@linaro.org; phi...@linaro.org; > > > peter.mayd...@linaro.org; quic_mathb...@quicinc.com; > > > stefa...@redhat.com; a...@rev.ng; a...@rev.ng; > > > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com > > > Subject: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > > > > > The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename > > > the identifiers to avoid shadowing the type name. > > > > > > The global `cpu_env` is shadowed by local `cpu_env` arguments, so we > > > rename the function arguments to avoid shadowing the global. > > > > > > Signed-off-by: Brian Cain > > > --- > > > target/hexagon/genptr.c | 56 - > > > target/hexagon/genptr.h | 18 > > > target/hexagon/mmvec/system_ext_mmvec.c | 4 +- > > > target/hexagon/mmvec/system_ext_mmvec.h | 2 +- > > > target/hexagon/op_helper.c | 4 +- > > > 5 files changed, 42 insertions(+), 42 deletions(-) > > > > > > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index > > > 217bc7bb5a..11377ac92b 100644 > > > --- a/target/hexagon/genptr.c > > > +++ b/target/hexagon/genptr.c > > > @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, > > TCGv > > > src) > > > tcg_gen_deposit_i64(result, result, src64, N * 8, 8); } > > > > > > -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int > > > mem_index) > > > +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int > > > +mem_index) > > > > I'd recommend moving both the type and the arg name to the new line, > > also indent the new line. > > static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, > > int > > mem_index) > > > > > I could be mistaken but AFAICT none of these lines are wrapped in the way > they're quoted above in my patch (nor the baseline). I don't think any of > these lines exceed 80 columns, so they shouldn't need wrapping, either. > > I double checked how it's displayed at the archive > https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01667.html to > make sure that it wasn't a misconfiguration of my mailer. For another > perspective - refer to the commit used to create this patch: > https://github.com/quic/qemu/commit/7f20565d403d16337ab6d69ee663121 > a3eef71e6 > > Is your review comment that "these lines should be wrapped and when you > do, make sure you do it like this"? Or "if you are going to wrap them, wrap > them like this"? Or something else? Yes. It looked like some adding the v_ would sometimes put the line over the 80 character size. If so, wrap them as described. If not, no wrapping is needed. > > > Otherwise, > > Reviewed-by: Taylor Simpson > >
RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> -Original Message- > From: Brian Cain > Sent: Thursday, October 5, 2023 4:22 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; arm...@redhat.com; richard.hender...@linaro.org; > phi...@linaro.org; peter.mayd...@linaro.org; quic_mathb...@quicinc.com; > stefa...@redhat.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com > Subject: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the > identifiers to avoid shadowing the type name. > > The global `cpu_env` is shadowed by local `cpu_env` arguments, so we > rename the function arguments to avoid shadowing the global. > > Signed-off-by: Brian Cain > --- > target/hexagon/genptr.c | 56 - > target/hexagon/genptr.h | 18 > target/hexagon/mmvec/system_ext_mmvec.c | 4 +- > target/hexagon/mmvec/system_ext_mmvec.h | 2 +- > target/hexagon/op_helper.c | 4 +- > 5 files changed, 42 insertions(+), 42 deletions(-) > > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index > 217bc7bb5a..11377ac92b 100644 > --- a/target/hexagon/genptr.c > +++ b/target/hexagon/genptr.c > @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv > src) > tcg_gen_deposit_i64(result, result, src64, N * 8, 8); } > > -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int > mem_index) > +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int > +mem_index) I'd recommend moving both the type and the arg name to the new line, also indent the new line. static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int mem_index) > > -static inline void gen_load_locked8u(TCGv_i64 dest, TCGv vaddr, int > mem_index) > +static inline void gen_load_locked8u(TCGv_i64 dest, TCGv v_addr, int > +mem_index) Ditto > static inline void gen_store_conditional4(DisasContext *ctx, > - TCGv pred, TCGv vaddr, TCGv src) > + TCGv pred, TCGv v_addr, TCGv > + src) Ditto > zero = tcg_constant_tl(0); > @@ -374,13 +374,13 @@ static inline void > gen_store_conditional4(DisasContext *ctx, } > > static inline void gen_store_conditional8(DisasContext *ctx, > - TCGv pred, TCGv vaddr, TCGv_i64 > src) > + TCGv pred, TCGv v_addr, > + TCGv_i64 src) Indent > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int > slot) > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int > +slot) Ditto > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int > slot); > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int > +slot); Ditto Otherwise, Reviewed-by: Taylor Simpson
RE: [PATCH v2 2/3] target/hexagon: fix some occurrences of -Wshadow=local
> -Original Message-
> From: Brian Cain
> Sent: Thursday, October 5, 2023 4:22 PM
> To: qemu-devel@nongnu.org
> Cc: bc...@quicinc.com; arm...@redhat.com; richard.hender...@linaro.org;
> phi...@linaro.org; peter.mayd...@linaro.org; quic_mathb...@quicinc.com;
> stefa...@redhat.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com
> Subject: [PATCH v2 2/3] target/hexagon: fix some occurrences of -
> Wshadow=local
>
> Of the changes in this commit, the changes in
> `HELPER(commit_hvx_stores)()` are less obvious. They are required because
> of some macro invocations like SCATTER_OP_WRITE_TO_MEM().
>
> e.g.:
>
> In file included from ../target/hexagon/op_helper.c:31:
> ../target/hexagon/mmvec/macros.h:205:18: error: declaration of ‘i’
> shadows a previous local [-Werror=shadow=compatible-local]
> 205 | for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE))
> { \
> | ^
> ../target/hexagon/op_helper.c:157:17: note: in expansion of macro
> ‘SCATTER_OP_WRITE_TO_MEM’
> 157 | SCATTER_OP_WRITE_TO_MEM(uint16_t);
> | ^~~
> ../target/hexagon/op_helper.c:135:9: note: shadowed declaration is here
> 135 | int i;
> | ^
> In file included from ../target/hexagon/op_helper.c:31:
> ../target/hexagon/mmvec/macros.h:204:19: error: declaration of ‘ra’
> shadows a previous local [-Werror=shadow=compatible-local]
> 204 | uintptr_t ra = GETPC(); \
> | ^~
> ../target/hexagon/op_helper.c:160:17: note: in expansion of macro
> ‘SCATTER_OP_WRITE_TO_MEM’
> 160 | SCATTER_OP_WRITE_TO_MEM(uint32_t);
> | ^~~
> ../target/hexagon/op_helper.c:134:15: note: shadowed declaration is here
> 134 | uintptr_t ra = GETPC();
> | ^~
>
> Reviewed-by: Matheus Tavares Bernardino
> Signed-off-by: Brian Cain
> ---
> target/hexagon/imported/alu.idef | 6 +++---
> target/hexagon/mmvec/macros.h| 6 +++---
> target/hexagon/op_helper.c | 9 +++--
> target/hexagon/translate.c | 9 -
> 4 files changed, 13 insertions(+), 17 deletions(-)
Reviewed-by: Taylor Simpson
RE: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Thursday, July 6, 2023 5:23 AM > To: ltaylorsimp...@gmail.com > Cc: bc...@quicinc.com; qemu-devel@nongnu.org; > quic_mathb...@quicinc.com; quic_mlie...@quicinc.com; > richard.hender...@linaro.org > Subject: Re: [PATCH v2] Hexagon: move GETPC() calls to top level helpers > > > > ltaylorsimp...@gmail.com wrote: > > > > > -Original Message- > > > From: Matheus Tavares Bernardino > > > Sent: Wednesday, July 5, 2023 12:35 PM > > > To: qemu-devel@nongnu.org > > > Cc: quic_mathb...@quicinc.com; bc...@quicinc.com; > > > ltaylorsimp...@gmail.com; quic_mlie...@quicinc.com; > > > richard.hender...@linaro.org > > > Subject: [PATCH v2] Hexagon: move GETPC() calls to top level helpers > > > > > > a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index > > > 8f3764d15e..7744e819ef 100644 > > > --- a/target/hexagon/op_helper.h > > > +++ b/target/hexagon/op_helper.h > > > +void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1, > > > + uint32_t slot, target_ulong vaddr, int size, > > > +uintptr_t ra); > > > > Are you sure this needs to be non-static? > > Yeah, since we remove the mem_load*() functions, check_noshuf() must > now be visible to the other compilation units that include macros.h, as we will > expand the fLOAD() macro to call it. Since the generated helper functions are included at the bottom of op_helper.c #include "helper_funcs_generated.c.inc" it can be static. It needs to be guarded with #ifndef CONFIG_HEXAGON_IDEF_PARSER because it is not used when by any of the idef-parser functions. Thanks, Taylor
RE: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
> -Original Message-
> From: Matheus Tavares Bernardino
> Sent: Wednesday, July 5, 2023 12:35 PM
> To: qemu-devel@nongnu.org
> Cc: quic_mathb...@quicinc.com; bc...@quicinc.com;
> ltaylorsimp...@gmail.com; quic_mlie...@quicinc.com;
> richard.hender...@linaro.org
> Subject: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
>
> As docs/devel/loads-stores.rst states:
>
> ``GETPC()`` should be used with great care: calling
> it in other functions that are *not* the top level
> ``HELPER(foo)`` will cause unexpected behavior. Instead, the
> value of ``GETPC()`` should be read from the helper and passed
> if needed to the functions that the helper calls.
>
> Let's fix the GETPC() usage in Hexagon, making sure it's always called
from
> top level helpers and passed down to the places where it's needed. There
> are two snippets where that is not currently the case:
>
> - probe_store(), which is only called from two helpers, so it's easy to
> move GETPC() up.
>
> - mem_load*() functions, which are also called directly from helpers,
> but through the MEM_LOAD*() set of macros. Note that this are only
> used when compiling with --disable-hexagon-idef-parser.
>
> In this case, we also take this opportunity to simplify the code,
> unifying the mem_load*() functions.
>
> Signed-off-by: Matheus Tavares Bernardino
> ---
> v1:
> d40fabcf9d6e92e4cd8d6a144e9b2a9acf4580dc.1688420966.git.quic_mathber
> n...@quicinc.com
>
> Changes in v2:
> - Fixed wrong cpu_ld* unification from previous version.
> - Passed retaddr down to check_noshuf() and further, as Taylor
> suggested.
> - Reorganized macros for simplification.
>
> target/hexagon/macros.h| 19 ++--
> target/hexagon/op_helper.h | 11 ++- target/hexagon/op_helper.c | 62
> +++---
> 3 files changed, 29 insertions(+), 63 deletions(-)
>
> diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index
> 5451b061ee..e44a932434 100644
> --- a/target/hexagon/macros.h
> +++ b/target/hexagon/macros.h
> @@ -173,15 +173,6 @@
> #define fLOAD(NUM, SIZE, SIGN, EA, DST) \
> -DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA)
> +DST = (size##SIZE##SIGN##_t)({ \
> +check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
> +MEM_LOAD##SIZE(env, EA, GETPC()); \
> +})
> #endif
This should be formatted as
#define fLOAD(...) \
do { \
check_noshuf(...); \
DST = ...; \
} while (0)
> a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index
> 8f3764d15e..7744e819ef 100644
> --- a/target/hexagon/op_helper.h
> +++ b/target/hexagon/op_helper.h
> +void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
> + uint32_t slot, target_ulong vaddr, int size,
> +uintptr_t ra);
Are you sure this needs to be non-static?
Othersiwe
Reviewed-by: Taylor Simpson
RE: [PATCH] Hexagon: move GETPC() calls to top level helpers
-Original Message-
From: Matheus Tavares Bernardino
Sent: Monday, July 3, 2023 3:50 PM
To: qemu-devel@nongnu.org
Cc: bc...@quicinc.com; quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com
Subject: [PATCH] Hexagon: move GETPC() calls to top level helpers
As docs/devel/loads-stores.rst states:
``GETPC()`` should be used with great care: calling
it in other functions that are *not* the top level
``HELPER(foo)`` will cause unexpected behavior. Instead, the
value of ``GETPC()`` should be read from the helper and passed
if needed to the functions that the helper calls.
Let's fix the GETPC() usage in Hexagon, making sure it's always called from
top level helpers and passed down to the places where it's needed. There are
two snippets where that is not currently the case:
- probe_store(), which is only called from two helpers, so it's easy to
move GETPC() up.
- mem_load*() functions, which are also called directly from helpers,
but through the MEM_LOAD*() set of macros. Note that this are only
used when compiling with --disable-hexagon-idef-parser.
In this case, we also take this opportunity to simplify the code,
unifying the mem_load*() functions.
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/macros.h| 22 ++---
target/hexagon/op_helper.h | 11 ++--- target/hexagon/op_helper.c | 49
+++---
3 files changed, 25 insertions(+), 57 deletions(-)
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index
5451b061ee..efb8013912 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
+
+#define MEM_LOADn(SIZE, VA) ({ \
+check_noshuf(env, pkt_has_store_s1, slot, VA, SIZE); \
+cpu_ldub_data_ra(env, VA, GETPC()); \
+})
Note that check_noshuf calls HELPER(probe_noshuf_load) and
HELPER(commit_store). Both of those call GETPC() from within. So, you'll
need to pull the contents into separate functions that take ra as an
argument.
Does this pass the test suite? You are only using the SIZE parameter in
check_noshuf, but cpu_ldub_data_ra only reads a single byte.
+#define MEM_LOAD1s(VA) ((int8_t)MEM_LOADn(1, VA)) #define
+MEM_LOAD1u(VA) ((uint8_t)MEM_LOADn(1, VA)) #define MEM_LOAD2s(VA)
+((int16_t)MEM_LOADn(2, VA)) #define MEM_LOAD2u(VA)
+((uint16_t)MEM_LOADn(2, VA)) #define MEM_LOAD4s(VA)
+((int32_t)MEM_LOADn(4, VA)) #define MEM_LOAD4u(VA)
+((uint32_t)MEM_LOADn(4, VA)) #define MEM_LOAD8s(VA)
+((int64_t)MEM_LOADn(8, VA)) #define MEM_LOAD8u(VA)
+((uint64_t)MEM_LOADn(8, VA))
A further cleanup would be to remove these altogether and modify the
definition of fLOAD to use MEM_LOADn.
#define MEM_STORE1(VA, DATA, SLOT) log_store32(env, VA, DATA, 1, SLOT)
#define MEM_STORE2(VA, DATA, SLOT) log_store32(env, VA, DATA, 2, SLOT) diff
--git a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index
8f3764d15e..845c3d197e 100644
--- a/target/hexagon/op_helper.h
+++ b/target/hexagon/op_helper.h
@@ -19,15 +19,8 @@
#define HEXAGON_OP_HELPER_H
+void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
+ uint32_t slot, target_ulong vaddr, int size);
Does this really need to be non-static?
Thanks,
Taylor
