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

[Alex Bennée]

Taylor Simpson <tsimpson@quicinc.com> writes:

>> -----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) {
>     ...
> }

You can go the whole hog and wrap everything up to minimise the chance
of functional changes in your debug legs in the main code, e.g.:

  #define tlb_debug(fmt, ...) do { \
      if (DEBUG_TLB_LOG_GATE) { \
          qemu_log_mask(CPU_LOG_MMU, "%s: " fmt, __func__, \
                        ## __VA_ARGS__); \
      } else if (DEBUG_TLB_GATE) { \
          fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \
      } \
  } while (0)

Then a statement like:

  tlb_debug("mmu_idx:0x%04" PRIx16 "\n", asked);

is unambiguously:

  - obviously a debug statement
  - always compiled, hence no bit rot
  - optimises away when gates are 0
  - doesn't accidentally include changes in behaviour

>
>
> Thanks,
> Taylor


-- 
Alex Bennée