RE: [PATCH v8 26/35] Hexagon (target/hexagon) TCG generation

[Taylor Simpson]

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On
> Behalf Of Philippe Mathieu-Daudé
> Sent: Thursday, February 11, 2021 6:23 PM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: richard.henderson@linaro.org; alex.bennee@linaro.org;
> laurent@vivier.eu; ale@rev.ng; Brian Cain <bcain@quicinc.com>
> Subject: Re: [PATCH v8 26/35] Hexagon (target/hexagon) TCG generation
>
> Hi Taylor,
>
> On 2/8/21 6:46 AM, Taylor Simpson wrote:
> > Include the generated files and set up the data structures
> >
> > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> > ---
> >  target/hexagon/genptr.h |  25 ++++
> >  target/hexagon/genptr.c | 331
> ++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 356 insertions(+)
> >  create mode 100644 target/hexagon/genptr.h
> >  create mode 100644 target/hexagon/genptr.c
> >
> > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
> > new file mode 100644
> > index 0000000..7481f4c
> > --- /dev/null
> > +++ b/target/hexagon/genptr.c
> > @@ -0,0 +1,331 @@
> > +
> > +#define QEMU_GENERATE
>
> Maybe move this ...
>
> > +#include "qemu/osdep.h"
> > +#include "qemu/log.h"
> > +#include "cpu.h"
> > +#include "internal.h"
> > +#include "tcg/tcg-op.h"
> > +#include "insn.h"
> > +#include "opcodes.h"
> > +#include "translate.h"
>
> ... here with a comment:
>
> #define QEMU_GENERATE /* Used internally by macros.h */
>
> > +#include "macros.h"
>
> and undef?
>
> #undef QEMU_GENERATE

OK

> > +#include "gen_tcg.h"
> > +
> > +static inline TCGv gen_read_preg(TCGv pred, uint8_t num)
> > +{
> > +    tcg_gen_mov_tl(pred, hex_pred[num]);
> > +    return pred;
> > +}
> > +
> > +static inline void gen_log_predicated_reg_write(int rnum, TCGv val, int
> slot)
> > +{
> > +    TCGv one = tcg_const_tl(1);
> > +    TCGv zero = tcg_const_tl(0);
> > +    TCGv slot_mask = tcg_temp_new();
> > +
> > +    tcg_gen_andi_tl(slot_mask, hex_slot_cancelled, 1 << slot);
> > +    tcg_gen_movcond_tl(TCG_COND_EQ, hex_new_value[rnum],
> slot_mask, zero,
> > +                           val, hex_new_value[rnum]);
> > +#if HEX_DEBUG
>
> Can you declare an 'bool hexagon_debug_enabled(void);' eventually
> inlined, so we can get this code compiled (to avoid bitroting) using
>
>   if (hexagon_debug_enabled()) {
>
> > +    /* Do this so HELPER(debug_commit_end) will know */
> > +    tcg_gen_movcond_tl(TCG_COND_EQ, hex_reg_written[rnum],
> slot_mask, zero,
> > +                       one, hex_reg_written[rnum]);
>
>   }
>
> > +#endif

Do we really need a function?  Why not change

#if HEX_DEBUG
    ...
#endif

to

if (HEX_DEBUG) {
    ...
}


Thanks,
Taylor