RE: [PATCH v7 05/13] target/hexagon: introduce new helper functions

From: Taylor Simpson <tsimpson@quicinc.com>
To: Anton Johansson <anjo@rev.ng>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: "ale@rev.ng" <ale@rev.ng>, Brian Cain <bcain@quicinc.com>,
	"richard.henderson@linaro.org" <richard.henderson@linaro.org>,
	"babush@rev.ng" <babush@rev.ng>, "nizzo@rev.ng" <nizzo@rev.ng>
Subject: RE: [PATCH v7 05/13] target/hexagon: introduce new helper functions
Date: Wed, 22 Dec 2021 19:29:09 +0000	[thread overview]
Message-ID: <SN4PR0201MB880814522737BC97CD21ED29DE7D9@SN4PR0201MB8808.namprd02.prod.outlook.com> (raw)
In-Reply-To: <20211217090129.23242-6-anjo@rev.ng>

> -----Original Message-----
> From: Anton Johansson <anjo@rev.ng>
> Sent: Friday, December 17, 2021 2:01 AM
> To: qemu-devel@nongnu.org
> Cc: ale@rev.ng; Taylor Simpson <tsimpson@quicinc.com>; Brian Cain
> <bcain@quicinc.com>; babush@rev.ng; nizzo@rev.ng;
> richard.henderson@linaro.org
> Subject: [PATCH v7 05/13] target/hexagon: introduce new helper functions
> 
> From: Niccolò Izzo <nizzo@rev.ng>
> 
> These helpers will be employed by the idef-parser generated code.
> "Helper" can here mean two things, a helper in the QEMU sense added to
> `helper.h` and `op_helper.c`, but also helper functions providing a manual
> TCG implementation of a certain features.
> 
> Signed-off-by: Alessandro Di Federico <ale@rev.ng>
> Signed-off-by: Niccolò Izzo <nizzo@rev.ng>
> Signed-off-by: Anton Johansson <anjo@rev.ng>
> ---
>  target/hexagon/genptr.c    | 166
> +++++++++++++++++++++++++++++++++++--
>  target/hexagon/genptr.h    |  16 +++-
>  target/hexagon/helper.h    |   2 +
>  target/hexagon/macros.h    |   9 ++
>  target/hexagon/op_helper.c |  10 +++
>  5 files changed, 195 insertions(+), 8 deletions(-)
> 
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> 
> +void gen_sat_i64(TCGv_i64 dest, TCGv_i64 source, int width) {
> +    TCGv_i64 max_val = tcg_const_i64((1 << (width - 1)) - 1);
> +    TCGv_i64 min_val = tcg_const_i64(-(1 << (width - 1)));

Doing those calculations as 32-bit numbers could be risky.  Either do the calculations in 64-bits (1LL << (width -1) -1LL) or assert that width <= 32.

Also, consider changing all the tcg_const_* to tcg_constant_*.  This is new in TCG and lets you avoid the tcg_temp_free at the end.

> +    tcg_gen_smin_i64(dest, source, max_val);
> +    tcg_gen_smax_i64(dest, dest, min_val);
> +    tcg_temp_free_i64(max_val);
> +    tcg_temp_free_i64(min_val);
> +}
> +
> +void gen_satu_i64(TCGv_i64 dest, TCGv_i64 source, int width) {
> +    TCGv_i64 max_val = tcg_const_i64((1 << width) - 1);

Same comment about this constant.

> +    tcg_gen_movcond_i64(TCG_COND_GTU, dest, source, max_val,
> max_val, source);
> +    TCGv_i64 zero = tcg_const_i64(0);

QEMU coding conventions call for declarations to be at the top of the function, not in the middle.

> +    tcg_gen_movcond_i64(TCG_COND_LT, dest, source, zero, zero, dest);
> +    tcg_temp_free_i64(max_val);
> +    tcg_temp_free_i64(zero);
> +}
> +
> +void gen_satu_i64_ovfl(TCGv ovfl, TCGv_i64 dest, TCGv_i64 source, int
> +width) {
> +    gen_sat_i64(dest, source, width);

gen_satu_i64

> +    TCGv_i64 ovfl_64 = tcg_temp_new_i64();
> +    tcg_gen_setcond_i64(TCG_COND_NE, ovfl_64, dest, source);
> +    tcg_gen_trunc_i64_tl(ovfl, ovfl_64);
> +    tcg_temp_free_i64(ovfl_64);
> +}
> +
> +/* Implements the fADDSAT64 macro in TCG */ void
> +gen_add_sat_i64(TCGv_i64 ret, TCGv_i64 a, TCGv_i64 b) {
> +    TCGv_i64 sum = tcg_temp_local_new_i64();
> +    tcg_gen_add_i64(sum, a, b);
> +
> +    TCGv_i64 xor = tcg_temp_new_i64();
> +    tcg_gen_xor_i64(xor, a, b);
> +
> +    TCGv_i64 mask = tcg_constant_i64(0x8000000000000000ULL);
> +
> +    TCGv_i64 cond1 = tcg_temp_local_new_i64();

This can be just tcg_temp_new_i64.

> +    tcg_gen_and_i64(cond1, xor, mask);
> +    tcg_temp_free_i64(xor);
> +
> +    TCGv_i64 cond2 = tcg_temp_local_new_i64();
> +    tcg_gen_xor_i64(cond2, a, sum);
> +    tcg_gen_and_i64(cond2, cond2, mask);
> +
> +    TCGLabel *no_ovfl_label = gen_new_label();
> +    TCGLabel *ovfl_label = gen_new_label();
> +    TCGLabel *ret_label = gen_new_label();
> +
> +    tcg_gen_brcondi_i64(TCG_COND_NE, cond1, 0, no_ovfl_label);
> +    tcg_temp_free_i64(cond1);
> +    tcg_gen_brcondi_i64(TCG_COND_NE, cond2, 0, ovfl_label);
> +    tcg_temp_free_i64(cond2);
> +    tcg_gen_br(no_ovfl_label);

This is redundant since the label is just after the jump.

> +
> +    gen_set_label(no_ovfl_label);
> +    tcg_gen_mov_i64(ret, sum);
> +    tcg_gen_br(ret_label);
> +
> +    gen_set_label(ovfl_label);
> +    TCGv_i64 cond3 = tcg_temp_new_i64();
> +    tcg_gen_and_i64(cond3, sum, mask);
> +    tcg_temp_free_i64(mask);
> +    tcg_temp_free_i64(sum);
> +    TCGv_i64 max_pos = tcg_constant_i64(0x7FFFFFFFFFFFFFFFLL);
> +    TCGv_i64 max_neg = tcg_constant_i64(0x8000000000000000LL);
> +    TCGv_i64 zero = tcg_constant_i64(0);
> +    tcg_gen_movcond_i64(TCG_COND_NE, ret, cond3, zero, max_pos,
> max_neg);
> +    tcg_temp_free_i64(max_pos);
> +    tcg_temp_free_i64(max_neg);
> +    tcg_temp_free_i64(zero);
> +    tcg_temp_free_i64(cond3);
> +    SET_USR_FIELD(USR_OVF, 1);
> +    tcg_gen_br(ret_label);

This is also redundant.

> +
> +    gen_set_label(ret_label);
> +}
> +

> diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
> index 722a115007..fc3844c8d1 100644
> --- a/target/hexagon/op_helper.c
> +++ b/target/hexagon/op_helper.c
> @@ -341,6 +341,16 @@ uint32_t HELPER(fbrev)(uint32_t addr)
>      return deposit32(addr, 0, 16, revbit16(addr));  }
> 
> +uint32_t HELPER(fbrev_32)(uint32_t addr) {
> +    return revbit32(addr);
> +}
> +
> +uint64_t HELPER(fbrev_64)(uint64_t addr) {
> +    return revbit64(addr);
> +}
> +

These are only used in a handful of instructions.  It would be better to let those use the existing generator to create helpers for the full instruction.

Here are the instructions in question:
Q6INSN(S2_brev,	"Rd32=brev(Rs32)",   ATTRIBS(A_ARCHV2), "Bit Reverse",{RdV = fBREV_4(RsV);})
Q6INSN(S2_brevp,"Rdd32=brev(Rss32)", ATTRIBS(), "Bit Reverse",{RddV = fBREV_8(RssV);})
Q6INSN(S2_ct0,  "Rd32=ct0(Rs32)",    ATTRIBS(A_ARCHV2), "Count Trailing",{RdV = fCL1_4(~fBREV_4(RsV));})
Q6INSN(S2_ct1,  "Rd32=ct1(Rs32)",    ATTRIBS(A_ARCHV2), "Count Trailing",{RdV = fCL1_4(fBREV_4(RsV));})
Q6INSN(S2_ct0p, "Rd32=ct0(Rss32)",   ATTRIBS(), "Count Trailing",{RdV = fCL1_8(~fBREV_8(RssV));})
Q6INSN(S2_ct1p, "Rd32=ct1(Rss32)",   ATTRIBS(), "Count Trailing",{RdV = fCL1_8(fBREV_8(RssV));})
Q6INSN(A4_tlbmatch,"Pd4=tlbmatch(Rss32,Rt32)",ATTRIBS(A_NOTE_LATEPRED,A_RESTRICT_LATEPRED),
"Detect if a VA/ASID matches a TLB entry",
{
    fHIDE(size4u_t TLBHI; size4u_t TLBLO; size4u_t MASK; size4u_t SIZE;)
    MASK = 0x07ffffff;
    TLBLO = fGETUWORD(0,RssV);
    TLBHI = fGETUWORD(1,RssV);
    SIZE = fMIN(6,fCL1_4(~fBREV_4(TLBLO)));
    MASK &= (0xffffffff << 2*SIZE);
    PdV = f8BITSOF(fGETBIT(31,TLBHI) && ((TLBHI & MASK) == (RtV & MASK)));
	fHIDE(MARK_LATE_PRED_WRITE(PdN))
})