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

[Taylor Simpson <tsimpson@quicinc.com>]

> -----Original Message-----
> From: Alex Bennée <alex.bennee@linaro.org>
> Sent: Tuesday, April 6, 2021 4:20 AM
> To: Taylor Simpson <tsimpson@quicinc.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>; qemu-devel@nongnu.org;
> richard.henderson@linaro.org; laurent@vivier.eu; ale@rev.ng; Brian Cain
> <bcain@quicinc.com>
> Subject: Re: [PATCH v8 26/35] Hexagon (target/hexagon) TCG generation
>
> 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
> >>
> >> > +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

That approach works fine for code that prints things out.  In fact, we have HEX_DEBUG_LOG for that purpose.  I can rewrite it as you suggest so the args always get compiled.

In addition, there are a dozen or so places where we emit additional TCG code when HEX_DEBUG is set.  We print the results in HELPER(debug_commit_end).  In the example above, we would end up with

#define debug_predicated_reg_write(rnum, slot_mask, zero, one) \
    do { \
        if (HEX_DEBUG) { \
            tcg_gen_movcond_tl(TCG_COND_EQ, hex_reg_written[rnum], slot_mask, zero, \
                                                   one, hex_reg_written[rnum]); \
        } \
    } while (0)

IMO, the invocation
    debug_predicated_reg_write(rnum, slot_mask, zero, one);
would be harder to read because you have to look at two places to see what is going on.  There would be only one invocation, so it would be better to put the if (HEX_DEBUG) in place.


Taylor