RE: [RFC PATCH v3 30/34] Hexagon (target/hexagon) TCG for instructions with multiple definitions

[Taylor Simpson]

> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Monday, August 31, 2020 8:41 PM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi;
> aleksandar.m.mail@gmail.com; ale@rev.ng
> Subject: Re: [RFC PATCH v3 30/34] Hexagon (target/hexagon) TCG for
> instructions with multiple definitions
>
> On 8/31/20 4:10 PM, Taylor Simpson wrote:
> >
> >
> >> -----Original Message-----
> >> From: Richard Henderson <richard.henderson@linaro.org>
> >> Sent: Monday, August 31, 2020 1:20 PM
> >> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> >> Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi;
> >> aleksandar.m.mail@gmail.com; ale@rev.ng
> >> Subject: Re: [RFC PATCH v3 30/34] Hexagon (target/hexagon) TCG for
> >> instructions with multiple definitions
> >>
> >> The fGEN_TCG_A2_add macro does not require nor use that {...}
> argument.
> >
> > The fGEN_TCG_A2_add macro does need that argument, but there are
> cases that
> > do need it.  Here's an example from gen_tcg.h
> >     #define fGEN_TCG_L2_loadrub_pr(SHORTCODE)      SHORTCODE
> > This is explained in the README, but basically the argument is useful if we
> > can properly define the macros that it contains to generate TCG.
> We're certainly not going to be able to handle e.g. "+" or "if", so it is 
> going
> to work only for the most trivial of SHORTCODE.
>
> Though in fact loadrub_pr makes that grade...

The prior version of this series included all the overrides I've written to 
date.  To reduce the size of this version, I removed most of them and only left 
the ones that are essential for correct execution.  I plan to submit the others 
in subsequent series.  Anyway, there are >50 overrides of load/store 
instructions that leverage SHORTCODE.

> > IMO, we don't want the person who writes an override having to
> reproduce the
> > generated code.  Assuming we have a definition of fGEN_TCG_A2_add and
> we
> > have the generator intelligently expanding the macros, this is what will be
> > generated.
> You need to give me a better example that A2_add, then.  Because I see that
> being exactly one line, calling a helper that handles all instructions of the
> same format, passing tcg_gen_add_tl as a callback.

Here's a more complicated example for a predicated post-increment load

Static void generate_L2_ploadrit_pi(CPUHexagonState *env, DisasContext*cts, 
insn_t *insn, packet_t *pkt)
{
/* L2_ploadrit_pi */
TCGv EA = tcg_temp_local_new();
int PtN = insn->regno[0];
TCGv PtV = hex_pred[PtN];
int RdN = insn->regno[1];
TCGv RdV = tcg_temp_local_new();
if (!is_preloaded(ctx, RdN)) {
    tcg_gen_mov_tl(hex_hew_value[RdN], hex_gpr[RdN]);
}
int RxN = insn->regno[2];
TCGv RxV = tcg_temp_local_new();
if (!is_preloaded(ctx, RxN)) {
    tcg_gen_mov_tl(hex_new_value[RdN], hex_gpr[RxN]);
}
int siV = insn->immed[0];
tcg_gen_mov_tl(RxV, hex_gpr[RxN]);
fGEN_TCG_L2_ploadrit_pi({fEA_REG(RxV); if(fLSBOLD(PtV)){ fPM_I(RxV,siV); 
fLOAD(1,4,u,EA,RdV);} else {LOAD_CANCEL(EA);}});
gen_log_reg_write(RdN, RdV, insn->slot, 1);
gen_log_reg_write(RxN, RxV, insn->slot, 1);
tcg_temp_free(EA);
tcg_temp_free(RdV);
tcg_temp_free(RxV);
/* L2_ploadrit_pi */
}


> Have a browse through my recent microblaze decodetree conversion.  Note
> that
> the basic logical operations are implemented with exactly one source line.

With a helper function, our compares are all one line also

static inline void gen_compare(TCGCond cond, TCGv res, TCGv arg1, TCGv arg2)
{
    TCGv one = tcg_const_tl(0xff);
    TCGv zero = tcg_const_tl(0);

    tcg_gen_movcond_tl(cond, res, arg1, arg2, one, zero);

    tcg_temp_free(one);
    tcg_temp_free(zero);
}

/* Compare instructions */
#define fGEN_TCG_C2_cmpeq(SHORTCODE) \
    gen_compare(TCG_COND_EQ, PdV, RsV, RtV)
#define fGEN_TCG_C4_cmpneq(SHORTCODE) \
    gen_compare(TCG_COND_NE, PdV, RsV, RtV)
#define fGEN_TCG_C2_cmpgt(SHORTCODE) \
    gen_compare(TCG_COND_GT, PdV, RsV, RtV)
#define fGEN_TCG_C2_cmpgtu(SHORTCODE) \
    gen_compare(TCG_COND_GTU, PdV, RsV, RtV)
...



> > Unlike the generate_<tag> functions that all have the same signature.  The
> overrides would have different signatures.  This would be more defensive
> programming because you know exactly where the variables come from but
> more verbose when writing the overrides by hand.  Also, note that these
> need to be macros in order to take advantage of the SHORTCODE.
> >
> > In other words, instead of
> > #define fGEN_TCG_A2_add(SHORTCODE)    tcg_gen_add_tl(RdV, RsV, RtV)
> >
> > We would write
> > #define fGEN_TCG_A2_add(env, ctx, insn, pkt, RdV, RsV, RtV,
> SHORTCODE)    tcg_gen_add_tl(RdV, RsV, RtV);
> >
> > Personally, I prefer the former, but will change to the latter if you feel
> strongly.
>
> This comes from trying to handle instructions in different ways, but
> represent
> them all the same.
>
> I guess I see the attraction of the magic non-parameters -- you get a
> compilation error if the variable is not present, but are not tied to
> positional parameters.
>
> Ho hum.  Maybe I'm trying to overthink this too much before tackling the
> ultimate goal of full parsing of the SHORTCODE.

Alessandro (ale@rev.ng) and Niccolo (nizzo@rev.ng) are working on this 😊

> Perhaps the only thing for the short term is to have the generator grep
> genptr.c for "#define fGEN", to choose between the two alternatives: inline
> generation or out-of-line helper generation.

That's certainly doable.  It will also be good to implement some of your other 
ideas
- Have the generator expand the DECL/READ/WRITE/FREE macros will make the 
generated code more readable and we can specialize them for predicated vs 
non-predicated instructions which will make translation faster.
- Generate the entire generate_<tag> function instead of just the body will 
make the generated code more readable.