[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v8 26/35] Hexagon (target/hexagon) TCG generation
From: |
Taylor Simpson |
Subject: |
RE: [PATCH v8 26/35] Hexagon (target/hexagon) TCG generation |
Date: |
Tue, 6 Apr 2021 15:44:39 +0000 |
> -----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