[PATCH v4 03/11] Hexagon (target/hexagon) Add overrides for S2_asr_r_r_sat/S2_asl_r_r_sat

Taylor Simpson posted 11 patches 3 years, 3 months ago
Maintainers: Taylor Simpson <tsimpson@quicinc.com>
There is a newer version of this series
[PATCH v4 03/11] Hexagon (target/hexagon) Add overrides for S2_asr_r_r_sat/S2_asl_r_r_sat
Posted by Taylor Simpson 3 years, 3 months ago
These instructions will not be generated by idef-parser, so we override
them manually.

Test cases added to tests/tcg/hexagon/usr.c

Co-authored-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
---
 target/hexagon/gen_tcg.h |  10 +++-
 target/hexagon/genptr.c  | 104 +++++++++++++++++++++++++++++++++++++++
 tests/tcg/hexagon/usr.c  |  30 ++++++++---
 3 files changed, 137 insertions(+), 7 deletions(-)

diff --git a/target/hexagon/gen_tcg.h b/target/hexagon/gen_tcg.h
index 50634ac459..b5fe22a07a 100644
--- a/target/hexagon/gen_tcg.h
+++ b/target/hexagon/gen_tcg.h
@@ -1,5 +1,5 @@
 /*
- *  Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved.
+ *  Copyright(c) 2019-2022 Qualcomm Innovation Center, Inc. All Rights Reserved.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -612,6 +612,14 @@
         tcg_temp_free(tmp); \
     } while (0)
 
+/* r0 = asr(r1, r2):sat */
+#define fGEN_TCG_S2_asr_r_r_sat(SHORTCODE) \
+    gen_asr_r_r_sat(RdV, RsV, RtV)
+
+/* r0 = asl(r1, r2):sat */
+#define fGEN_TCG_S2_asl_r_r_sat(SHORTCODE) \
+    gen_asl_r_r_sat(RdV, RsV, RtV)
+
 /* Floating point */
 #define fGEN_TCG_F2_conv_sf2df(SHORTCODE) \
     gen_helper_conv_sf2df(RddV, cpu_env, RsV)
diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index 85416dd530..ccfed9d643 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -456,6 +456,110 @@ static TCGv gen_8bitsof(TCGv result, TCGv value)
     return result;
 }
 
+/* Shift left with saturation */
+static void gen_shl_sat(TCGv dst, TCGv src, TCGv shift_amt)
+{
+    TCGv_i64 src64 = tcg_temp_local_new_i64();
+    TCGv_i64 shift64 = tcg_temp_new_i64();
+    TCGv_i64 dst64 = tcg_temp_new_i64();
+    TCGv dst_sar = tcg_temp_new();
+    TCGv ovf = tcg_temp_new();
+    TCGv satval = tcg_temp_new();
+    TCGv min = tcg_constant_tl(0x80000000);
+    TCGv max = tcg_constant_tl(0x7fffffff);
+
+    /*
+     *     dst64 = (int64_t)src << (int64_t)shift_amt
+     *     dst = (int32_t)dst64
+     *     dst_sar = dst >> shift_amt
+     *     if (dst_sar != src) {
+     *         usr.OVF = 1
+     *         dst = src < 0 ? min : max
+     *     }
+     */
+    tcg_gen_ext_i32_i64(src64, src);
+    tcg_gen_ext_i32_i64(shift64, shift_amt);
+    tcg_gen_shl_i64(dst64, src64, shift64);
+
+    tcg_gen_extrl_i64_i32(dst, dst64);
+    tcg_gen_sar_tl(dst_sar, dst, shift_amt);
+
+    tcg_gen_setcond_tl(TCG_COND_NE, ovf, dst_sar, src);
+    tcg_gen_shli_tl(ovf, ovf, reg_field_info[USR_OVF].offset);
+    tcg_gen_or_tl(hex_new_value[HEX_REG_USR], hex_new_value[HEX_REG_USR], ovf);
+
+    tcg_gen_movcond_tl(TCG_COND_LT, satval, src, tcg_constant_tl(0),
+                       min, max);
+    tcg_gen_movcond_tl(TCG_COND_EQ, dst, dst_sar, src, dst, satval);
+
+    tcg_temp_free_i64(src64);
+    tcg_temp_free_i64(shift64);
+    tcg_temp_free_i64(dst64);
+    tcg_temp_free(dst_sar);
+    tcg_temp_free(ovf);
+    tcg_temp_free(satval);
+}
+
+static void gen_sar(TCGv dst, TCGv src, TCGv shift_amt)
+{
+    /*
+     *  Shift arithmetic right
+     *  Robust when shift_amt is >31 bits
+     */
+    TCGv tmp = tcg_temp_new();
+    tcg_gen_umin_tl(tmp, shift_amt, tcg_constant_tl(31));
+    tcg_gen_sar_tl(dst, src, tmp);
+    tcg_temp_free(tmp);
+}
+
+/* Bidirectional shift right with saturation */
+static void gen_asr_r_r_sat(TCGv RdV, TCGv RsV, TCGv RtV)
+{
+    TCGv shift_amt = tcg_temp_local_new();
+    TCGLabel *positive = gen_new_label();
+    TCGLabel *done = gen_new_label();
+
+    tcg_gen_sextract_i32(shift_amt, RtV, 0, 7);
+    tcg_gen_brcondi_tl(TCG_COND_GE, shift_amt, 0, positive);
+
+    /* Negative shift amount => shift left */
+    tcg_gen_neg_tl(shift_amt, shift_amt);
+    gen_shl_sat(RdV, RsV, shift_amt);
+    tcg_gen_br(done);
+
+    gen_set_label(positive);
+    /* Positive shift amount => shift right */
+    gen_sar(RdV, RsV, shift_amt);
+
+    gen_set_label(done);
+
+    tcg_temp_free(shift_amt);
+}
+
+/* Bidirectional shift left with saturation */
+static void gen_asl_r_r_sat(TCGv RdV, TCGv RsV, TCGv RtV)
+{
+    TCGv shift_amt = tcg_temp_local_new();
+    TCGLabel *positive = gen_new_label();
+    TCGLabel *done = gen_new_label();
+
+    tcg_gen_sextract_i32(shift_amt, RtV, 0, 7);
+    tcg_gen_brcondi_tl(TCG_COND_GE, shift_amt, 0, positive);
+
+    /* Negative shift amount => shift right */
+    tcg_gen_neg_tl(shift_amt, shift_amt);
+    gen_sar(RdV, RsV, shift_amt);
+    tcg_gen_br(done);
+
+    gen_set_label(positive);
+    /* Positive shift amount => shift left */
+    gen_shl_sat(RdV, RsV, shift_amt);
+
+    gen_set_label(done);
+
+    tcg_temp_free(shift_amt);
+}
+
 static intptr_t vreg_src_off(DisasContext *ctx, int num)
 {
     intptr_t offset = offsetof(CPUHexagonState, VRegs[num]);
diff --git a/tests/tcg/hexagon/usr.c b/tests/tcg/hexagon/usr.c
index fb4514989c..63c7d2700f 100644
--- a/tests/tcg/hexagon/usr.c
+++ b/tests/tcg/hexagon/usr.c
@@ -429,6 +429,7 @@ FUNC_P_OP_P(vabshsat,           "%0 = vabsh(%2):sat")
 FUNC_P_OP_PP(vnavgwr,           "%0 = vnavgw(%2, %3):rnd:sat")
 FUNC_R_OP_RI(round_ri_sat,      "%0 = round(%2, #%3):sat")
 FUNC_R_OP_RR(asr_r_r_sat,       "%0 = asr(%2, %3):sat")
+FUNC_R_OP_RR(asl_r_r_sat,       "%0 = asl(%2, %3):sat")
 
 FUNC_XPp_OP_PP(ACS,             "%0, p2 = vacsh(%3, %4)")
 
@@ -907,12 +908,29 @@ int main()
     TEST_R_OP_RI(round_ri_sat,         0x0000ffff, 2, 0x00004000, USR_CLEAR);
     TEST_R_OP_RI(round_ri_sat,         0x7fffffff, 2, 0x1fffffff, USR_OVF);
 
-    TEST_R_OP_RR(asr_r_r_sat,          0x0000ffff, 0x00000002, 0x00003fff,
-                 USR_CLEAR);
-    TEST_R_OP_RR(asr_r_r_sat,          0x00ffffff, 0xfffffff5, 0x7fffffff,
-                 USR_OVF);
-    TEST_R_OP_RR(asr_r_r_sat,          0x80000000, 0xfffffff5, 0x80000000,
-                 USR_OVF);
+    TEST_R_OP_RR(asr_r_r_sat,  0x0000ffff, 0x02, 0x00003fff, USR_CLEAR);
+    TEST_R_OP_RR(asr_r_r_sat,  0x80000000, 0x01, 0xc0000000, USR_CLEAR);
+    TEST_R_OP_RR(asr_r_r_sat,  0xffffffff, 0x01, 0xffffffff, USR_CLEAR);
+    TEST_R_OP_RR(asr_r_r_sat,  0x00ffffff, 0xf5, 0x7fffffff, USR_OVF);
+    TEST_R_OP_RR(asr_r_r_sat,  0x80000000, 0xf5, 0x80000000, USR_OVF);
+    TEST_R_OP_RR(asr_r_r_sat,  0x7fff0000, 0x42, 0x7fffffff, USR_OVF);
+    TEST_R_OP_RR(asr_r_r_sat,  0xff000000, 0x42, 0x80000000, USR_OVF);
+    TEST_R_OP_RR(asr_r_r_sat,        4096,   32, 0x00000000, USR_CLEAR);
+    TEST_R_OP_RR(asr_r_r_sat,        4096,  -32, 0x7fffffff, USR_OVF);
+    TEST_R_OP_RR(asr_r_r_sat,       -4096,   32, 0xffffffff, USR_CLEAR);
+    TEST_R_OP_RR(asr_r_r_sat,       -4096,  -32, 0x80000000, USR_OVF);
+
+    TEST_R_OP_RR(asl_r_r_sat,  0x00000000, 0x40, 0x00000000, USR_CLEAR);
+    TEST_R_OP_RR(asl_r_r_sat,  0x80000000, 0xff, 0xc0000000, USR_CLEAR);
+    TEST_R_OP_RR(asl_r_r_sat,  0xffffffff, 0xff, 0xffffffff, USR_CLEAR);
+    TEST_R_OP_RR(asl_r_r_sat,  0x00ffffff, 0x0b, 0x7fffffff, USR_OVF);
+    TEST_R_OP_RR(asl_r_r_sat,  0x80000000, 0x0b, 0x80000000, USR_OVF);
+    TEST_R_OP_RR(asl_r_r_sat,  0x7fff0000, 0xbe, 0x7fffffff, USR_OVF);
+    TEST_R_OP_RR(asl_r_r_sat,  0xff000000, 0xbe, 0x80000000, USR_OVF);
+    TEST_R_OP_RR(asl_r_r_sat,        4096,   32, 0x7fffffff, USR_OVF);
+    TEST_R_OP_RR(asl_r_r_sat,        4096,  -32, 0x00000000, USR_CLEAR);
+    TEST_R_OP_RR(asl_r_r_sat,       -4096,   32, 0x80000000, USR_OVF);
+    TEST_R_OP_RR(asl_r_r_sat,       -4096,  -32, 0xffffffff, USR_CLEAR);
 
     TEST_XPp_OP_PP(ACS, 0x0004000300020001ULL, 0x0001000200030004ULL,
                    0x0000000000000000ULL, 0x0004000300030004ULL, 0xf0,
-- 
2.17.1

Re: [PATCH v4 03/11] Hexagon (target/hexagon) Add overrides for S2_asr_r_r_sat/S2_asl_r_r_sat
Posted by Richard Henderson 3 years, 3 months ago
On 11/8/22 15:05, Taylor Simpson wrote:
> +/* Shift left with saturation */
> +static void gen_shl_sat(TCGv dst, TCGv src, TCGv shift_amt)
> +{
> +    TCGv_i64 src64 = tcg_temp_local_new_i64();
> +    TCGv_i64 shift64 = tcg_temp_new_i64();
> +    TCGv_i64 dst64 = tcg_temp_new_i64();
> +    TCGv dst_sar = tcg_temp_new();
> +    TCGv ovf = tcg_temp_new();
> +    TCGv satval = tcg_temp_new();
> +    TCGv min = tcg_constant_tl(0x80000000);
> +    TCGv max = tcg_constant_tl(0x7fffffff);
> +
> +    /*
> +     *     dst64 = (int64_t)src << (int64_t)shift_amt
> +     *     dst = (int32_t)dst64
> +     *     dst_sar = dst >> shift_amt
> +     *     if (dst_sar != src) {
> +     *         usr.OVF = 1
> +     *         dst = src < 0 ? min : max
> +     *     }
> +     */
> +    tcg_gen_ext_i32_i64(src64, src);
> +    tcg_gen_ext_i32_i64(shift64, shift_amt);
> +    tcg_gen_shl_i64(dst64, src64, shift64);
> +
> +    tcg_gen_extrl_i64_i32(dst, dst64);
> +    tcg_gen_sar_tl(dst_sar, dst, shift_amt);

I don't think this is quite right.  In particular:

> +static void gen_asr_r_r_sat(TCGv RdV, TCGv RsV, TCGv RtV)
> +{
> +    TCGv shift_amt = tcg_temp_local_new();
> +    TCGLabel *positive = gen_new_label();
> +    TCGLabel *done = gen_new_label();
> +
> +    tcg_gen_sextract_i32(shift_amt, RtV, 0, 7);

This suggests shift amounts -64 ... 63.

> +    tcg_gen_brcondi_tl(TCG_COND_GE, shift_amt, 0, positive);
> +
> +    /* Negative shift amount => shift left */
> +    tcg_gen_neg_tl(shift_amt, shift_amt);

-64 -> 64.

So!  We have two out-of-range shifts in gen_shl_sat, both i64 and i32.
If we fix one, then we don't even need the extension to i64 either.

Consider

     /*
      * sh32 = shift & 31;
      * dst = sh32 == shift ? src : 0;
      * dst <<= sh32;
      * dst_sar = dst >> sh32;
      * if (dst_sar != src) ...
      */
     tcg_gen_andi_i32(sh32, shift_amt, 31);
     tcg_gen_movcond_i32(TCG_COND_EQ, dst,
                         sh32, shift_amt,
                         src, tcg_constant_i32(0));
     tcg_gen_shl_i32(dst, dst, sh32);
     tcg_gen_sar_i32(dst_sar, dst, sh32);


r~
RE: [PATCH v4 03/11] Hexagon (target/hexagon) Add overrides for S2_asr_r_r_sat/S2_asl_r_r_sat
Posted by Taylor Simpson 3 years, 3 months ago

> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Tuesday, November 8, 2022 1:20 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@linaro.org; ale@rev.ng; anjo@rev.ng; Brian Cain
> <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>
> Subject: Re: [PATCH v4 03/11] Hexagon (target/hexagon) Add overrides for
> S2_asr_r_r_sat/S2_asl_r_r_sat
> 
> Consider
> 
>      /*
>       * sh32 = shift & 31;
>       * dst = sh32 == shift ? src : 0;
>       * dst <<= sh32;
>       * dst_sar = dst >> sh32;
>       * if (dst_sar != src) ...
>       */
>      tcg_gen_andi_i32(sh32, shift_amt, 31);
>      tcg_gen_movcond_i32(TCG_COND_EQ, dst,
>                          sh32, shift_amt,
>                          src, tcg_constant_i32(0));
>      tcg_gen_shl_i32(dst, dst, sh32);
>      tcg_gen_sar_i32(dst_sar, dst, sh32);

Will do.

Thanks,
Taylor