> -----Original Message-----
> From: Anton Johansson <a...@rev.ng>
> Sent: Friday, December 17, 2021 2:01 AM
> To: qemu-devel@nongnu.org
> Cc: a...@rev.ng; Taylor Simpson <tsimp...@quicinc.com>; Brian Cain
> <bc...@quicinc.com>; bab...@rev.ng; ni...@rev.ng;
> richard.hender...@linaro.org
> Subject: [PATCH v7 05/13] target/hexagon: introduce new helper functions
>
> From: Niccolò Izzo <ni...@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 <a...@rev.ng>
> Signed-off-by: Niccolò Izzo <ni...@rev.ng>
> Signed-off-by: Anton Johansson <a...@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))
})
