[PATCH v2] Hexagon (target/hexagon) Additional instructions handled by idef-parser

Taylor Simpson posted 1 patch 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230426173232.2227787-1-tsimpson@quicinc.com
Maintainers: Alessandro Di Federico <ale@rev.ng>, Anton Johansson <anjo@rev.ng>, Taylor Simpson <tsimpson@quicinc.com>
There is a newer version of this series
target/hexagon/idef-parser/parser-helpers.h |  2 +-
target/hexagon/idef-parser/parser-helpers.c | 37 ++++++++++----
tests/tcg/hexagon/fpstuff.c                 | 54 +++++++++++++++++++++
target/hexagon/gen_idef_parser_funcs.py     | 10 +++-
4 files changed, 91 insertions(+), 12 deletions(-)
[PATCH v2] Hexagon (target/hexagon) Additional instructions handled by idef-parser
Posted by Taylor Simpson 1 year ago
**** Changes in v2 ****
Fix bug in imm_print identified in clang build

Currently, idef-parser skips all floating point instructions.  However,
there are some floating point instructions that can be handled.

The following instructions are now parsed
    F2_sfimm_p
    F2_sfimm_n
    F2_dfimm_p
    F2_dfimm_n
    F2_dfmpyll
    F2_dfmpylh

To make these instructions work, we fix some bugs in parser-helpers.c
    gen_rvalue_extend
    gen_cast_op
    imm_print

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

Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
---
 target/hexagon/idef-parser/parser-helpers.h |  2 +-
 target/hexagon/idef-parser/parser-helpers.c | 37 ++++++++++----
 tests/tcg/hexagon/fpstuff.c                 | 54 +++++++++++++++++++++
 target/hexagon/gen_idef_parser_funcs.py     | 10 +++-
 4 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/target/hexagon/idef-parser/parser-helpers.h b/target/hexagon/idef-parser/parser-helpers.h
index 1239d23a6a..7c58087169 100644
--- a/target/hexagon/idef-parser/parser-helpers.h
+++ b/target/hexagon/idef-parser/parser-helpers.h
@@ -80,7 +80,7 @@ void reg_compose(Context *c, YYLTYPE *locp, HexReg *reg, char reg_id[5]);
 
 void reg_print(Context *c, YYLTYPE *locp, HexReg *reg);
 
-void imm_print(Context *c, YYLTYPE *locp, HexImm *imm);
+void imm_print(Context *c, YYLTYPE *locp, HexValue *rvalue);
 
 void var_print(Context *c, YYLTYPE *locp, HexVar *var);
 
diff --git a/target/hexagon/idef-parser/parser-helpers.c b/target/hexagon/idef-parser/parser-helpers.c
index 86511efb62..deaedbb293 100644
--- a/target/hexagon/idef-parser/parser-helpers.c
+++ b/target/hexagon/idef-parser/parser-helpers.c
@@ -167,8 +167,9 @@ void reg_print(Context *c, YYLTYPE *locp, HexReg *reg)
     EMIT(c, "hex_gpr[%u]", reg->id);
 }
 
-void imm_print(Context *c, YYLTYPE *locp, HexImm *imm)
+void imm_print(Context *c, YYLTYPE *locp, HexValue *rvalue)
 {
+    HexImm *imm = &rvalue->imm;
     switch (imm->type) {
     case I:
         EMIT(c, "i");
@@ -177,7 +178,21 @@ void imm_print(Context *c, YYLTYPE *locp, HexImm *imm)
         EMIT(c, "%ciV", imm->id);
         break;
     case VALUE:
-        EMIT(c, "((int64_t) %" PRIu64 "ULL)", (int64_t) imm->value);
+        if (rvalue->bit_width == 32) {
+            if (rvalue->signedness == UNSIGNED) {
+                EMIT(c, "((uint32_t) %" PRIu32 ")", (uint32_t) imm->value);
+            }  else {
+                EMIT(c, "((int32_t) %" PRId32 ")", (int32_t) imm->value);
+            }
+        } else if (rvalue->bit_width == 64) {
+            if (rvalue->signedness == UNSIGNED) {
+                EMIT(c, "((uint64_t) %" PRIu64 "ULL)", (uint64_t) imm->value);
+            } else {
+                EMIT(c, "((int64_t) %" PRId64 "LL)", (int64_t) imm->value);
+            }
+        } else {
+            g_assert_not_reached();
+        }
         break;
     case QEMU_TMP:
         EMIT(c, "qemu_tmp_%" PRIu64, imm->index);
@@ -213,7 +228,7 @@ void rvalue_print(Context *c, YYLTYPE *locp, void *pointer)
       tmp_print(c, locp, &rvalue->tmp);
       break;
   case IMMEDIATE:
-      imm_print(c, locp, &rvalue->imm);
+      imm_print(c, locp, rvalue);
       break;
   case VARID:
       var_print(c, locp, &rvalue->var);
@@ -386,13 +401,10 @@ HexValue gen_rvalue_extend(Context *c, YYLTYPE *locp, HexValue *rvalue)
 
     if (rvalue->type == IMMEDIATE) {
         HexValue res = gen_imm_qemu_tmp(c, locp, 64, rvalue->signedness);
-        bool is_unsigned = (rvalue->signedness == UNSIGNED);
-        const char *sign_suffix = is_unsigned ? "u" : "";
         gen_c_int_type(c, locp, 64, rvalue->signedness);
-        OUT(c, locp, " ", &res, " = ");
-        OUT(c, locp, "(", sign_suffix, "int64_t) ");
-        OUT(c, locp, "(", sign_suffix, "int32_t) ");
-        OUT(c, locp, rvalue, ";\n");
+        OUT(c, locp, " ", &res, " = (");
+        gen_c_int_type(c, locp, 64, rvalue->signedness);
+        OUT(c, locp, ")", rvalue, ";\n");
         return res;
     } else {
         HexValue res = gen_tmp(c, locp, 64, rvalue->signedness);
@@ -963,7 +975,12 @@ HexValue gen_cast_op(Context *c,
     if (src->bit_width == target_width) {
         return *src;
     } else if (src->type == IMMEDIATE) {
-        HexValue res = *src;
+        HexValue res;
+        if (src->bit_width < target_width) {
+            res = gen_rvalue_extend(c, locp, src);
+        } else {
+            res = *src;
+        }
         res.bit_width = target_width;
         res.signedness = signedness;
         return res;
diff --git a/tests/tcg/hexagon/fpstuff.c b/tests/tcg/hexagon/fpstuff.c
index 90ce9a6ef3..28f9397155 100644
--- a/tests/tcg/hexagon/fpstuff.c
+++ b/tests/tcg/hexagon/fpstuff.c
@@ -20,6 +20,7 @@
  */
 
 #include <stdio.h>
+#include <float.h>
 
 const int FPINVF_BIT = 1;                 /* Invalid */
 const int FPINVF = 1 << FPINVF_BIT;
@@ -706,6 +707,57 @@ static void check_float2int_convs()
     check_fpstatus(usr, FPINVF);
 }
 
+static void check_float_consts(void)
+{
+    int res32;
+    unsigned long long res64;
+
+    asm("%0 = sfmake(#%1):neg\n\t" : "=r"(res32) : "i"(0xf));
+    check32(res32, 0xbc9e0000);
+
+    asm("%0 = sfmake(#%1):pos\n\t" : "=r"(res32) : "i"(0xf));
+    check32(res32, 0x3c9e0000);
+
+    asm("%0 = dfmake(#%1):neg\n\t" : "=r"(res64) : "i"(0xf));
+    check64(res64, 0xbf93c00000000000ULL);
+
+    asm("%0 = dfmake(#%1):pos\n\t" : "=r"(res64) : "i"(0xf));
+    check64(res64, 0x3f93c00000000000ULL);
+}
+
+static inline unsigned long long dfmpyll(double x, double y)
+{
+    unsigned long long res64;
+    asm("%0 = dfmpyll(%1, %2)" : "=r"(res64) : "r"(x), "r"(y));
+    return res64;
+}
+
+static inline unsigned long long dfmpylh(double acc, double x, double y)
+{
+    unsigned long long res64 = *(unsigned long long *)&acc;
+    asm("%0 += dfmpylh(%1, %2)" : "+r"(res64) : "r"(x), "r"(y));
+    return res64;
+}
+
+static void check_dfmpyxx(void)
+{
+    unsigned long long res64;
+
+    res64 = dfmpyll(DBL_MIN, DBL_MIN);
+    check64(res64, 0ULL);
+    res64 = dfmpyll(-1.0, DBL_MIN);
+    check64(res64, 0ULL);
+    res64 = dfmpyll(DBL_MAX, DBL_MAX);
+    check64(res64, 0x1fffffffdULL);
+
+    res64 = dfmpylh(DBL_MIN, DBL_MIN, DBL_MIN);
+    check64(res64, 0x10000000000000ULL);
+    res64 = dfmpylh(-1.0, DBL_MAX, DBL_MIN);
+    check64(res64, 0xc00fffffffe00000ULL);
+    res64 = dfmpylh(DBL_MAX, 0.0, -1.0);
+    check64(res64, 0x7fefffffffffffffULL);
+}
+
 int main()
 {
     check_compare_exception();
@@ -718,6 +770,8 @@ int main()
     check_sffixupd();
     check_sffms();
     check_float2int_convs();
+    check_float_consts();
+    check_dfmpyxx();
 
     puts(err ? "FAIL" : "PASS");
     return err ? 1 : 0;
diff --git a/target/hexagon/gen_idef_parser_funcs.py b/target/hexagon/gen_idef_parser_funcs.py
index afe68bdb6f..b766798ad5 100644
--- a/target/hexagon/gen_idef_parser_funcs.py
+++ b/target/hexagon/gen_idef_parser_funcs.py
@@ -103,7 +103,15 @@ def main():
                 continue
             if tag.startswith("V6_"):
                 continue
-            if tag.startswith("F"):
+            if ( tag.startswith("F") and
+                 tag not in {
+                     "F2_sfimm_p",
+                     "F2_sfimm_n",
+                     "F2_dfimm_p",
+                     "F2_dfimm_n",
+                     "F2_dfmpyll",
+                     "F2_dfmpylh"
+                 }):
                 continue
             if tag.endswith("_locked"):
                 continue
-- 
2.25.1

Re: [PATCH v2] Hexagon (target/hexagon) Additional instructions handled by idef-parser
Posted by Anton Johansson via 1 year ago
On 4/26/23 19:32, Taylor Simpson wrote:
> **** Changes in v2 ****
> Fix bug in imm_print identified in clang build
>
> Currently, idef-parser skips all floating point instructions.  However,
> there are some floating point instructions that can be handled.
>
> The following instructions are now parsed
>      F2_sfimm_p
>      F2_sfimm_n
>      F2_dfimm_p
>      F2_dfimm_n
>      F2_dfmpyll
>      F2_dfmpylh
>
> To make these instructions work, we fix some bugs in parser-helpers.c
>      gen_rvalue_extend
>      gen_cast_op
>      imm_print
>
> Test cases added to tests/tcg/hexagon/fpstuff.c
>
> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> ---
>   target/hexagon/idef-parser/parser-helpers.h |  2 +-
>   target/hexagon/idef-parser/parser-helpers.c | 37 ++++++++++----
>   tests/tcg/hexagon/fpstuff.c                 | 54 +++++++++++++++++++++
>   target/hexagon/gen_idef_parser_funcs.py     | 10 +++-
>   4 files changed, 91 insertions(+), 12 deletions(-)

I'm getting a harness failure on

     v65_Q6_R_mpy_RR_rnd.c

I'll take a deeper look at this next week.

diff:

  Intrinsic: ----------- Test Result Summary ---------- Word32 
Q6_R_mpy_RR_rnd(Word32 Rs, Word32 Rt)
-40000000 :  Q6_R_mpy_RR_rnd(INT32_MIN,INT32_MIN)
-1 :  Q6_R_mpy_RR_rnd(-1,INT32_MIN)
-0 :  Q6_R_mpy_RR_rnd(0,INT32_MIN)
-0 :  Q6_R_mpy_RR_rnd(1,INT32_MIN)
-c0000001 :  Q6_R_mpy_RR_rnd(INT32_MAX,INT32_MIN)
-1 :  Q6_R_mpy_RR_rnd(INT32_MIN,-1)
-0 :  Q6_R_mpy_RR_rnd(-1,-1)
-0 :  Q6_R_mpy_RR_rnd(0,-1)
-0 :  Q6_R_mpy_RR_rnd(1,-1)
-0 :  Q6_R_mpy_RR_rnd(INT32_MAX,-1)
-0 :  Q6_R_mpy_RR_rnd(INT32_MIN,0)
-0 :  Q6_R_mpy_RR_rnd(-1,0)
-0 :  Q6_R_mpy_RR_rnd(0,0)
-0 :  Q6_R_mpy_RR_rnd(1,0)
-0 :  Q6_R_mpy_RR_rnd(INT32_MAX,0)
-0 :  Q6_R_mpy_RR_rnd(INT32_MIN,1)
-0 :  Q6_R_mpy_RR_rnd(-1,1)
-0 :  Q6_R_mpy_RR_rnd(0,1)
-0 :  Q6_R_mpy_RR_rnd(1,1)
-0 :  Q6_R_mpy_RR_rnd(INT32_MAX,1)
-c0000001 :  Q6_R_mpy_RR_rnd(INT32_MIN,INT32_MAX)
-0 :  Q6_R_mpy_RR_rnd(-1,INT32_MAX)
-0 :  Q6_R_mpy_RR_rnd(0,INT32_MAX)
-0 :  Q6_R_mpy_RR_rnd(1,INT32_MAX)
-3fffffff :  Q6_R_mpy_RR_rnd(INT32_MAX,INT32_MAX)
+3fffffff :  Q6_R_mpy_RR_rnd(INT32_MIN,INT32_MIN)
+0 :  Q6_R_mpy_RR_rnd(-1,INT32_MIN)
+ffffffff :  Q6_R_mpy_RR_rnd(0,INT32_MIN)
+ffffffff :  Q6_R_mpy_RR_rnd(1,INT32_MIN)
+c0000000 :  Q6_R_mpy_RR_rnd(INT32_MAX,INT32_MIN)
+0 :  Q6_R_mpy_RR_rnd(INT32_MIN,-1)
+ffffffff :  Q6_R_mpy_RR_rnd(-1,-1)
+ffffffff :  Q6_R_mpy_RR_rnd(0,-1)
+ffffffff :  Q6_R_mpy_RR_rnd(1,-1)
+ffffffff :  Q6_R_mpy_RR_rnd(INT32_MAX,-1)
+ffffffff :  Q6_R_mpy_RR_rnd(INT32_MIN,0)
+ffffffff :  Q6_R_mpy_RR_rnd(-1,0)
+ffffffff :  Q6_R_mpy_RR_rnd(0,0)
+ffffffff :  Q6_R_mpy_RR_rnd(1,0)
+ffffffff :  Q6_R_mpy_RR_rnd(INT32_MAX,0)
+ffffffff :  Q6_R_mpy_RR_rnd(INT32_MIN,1)
+ffffffff :  Q6_R_mpy_RR_rnd(-1,1)
+ffffffff :  Q6_R_mpy_RR_rnd(0,1)
+ffffffff :  Q6_R_mpy_RR_rnd(1,1)
+ffffffff :  Q6_R_mpy_RR_rnd(INT32_MAX,1)
+c0000000 :  Q6_R_mpy_RR_rnd(INT32_MIN,INT32_MAX)
+ffffffff :  Q6_R_mpy_RR_rnd(-1,INT32_MAX)
+ffffffff :  Q6_R_mpy_RR_rnd(0,INT32_MAX)
+ffffffff :  Q6_R_mpy_RR_rnd(1,INT32_MAX)
+3ffffffe :  Q6_R_mpy_RR_rnd(INT32_MAX,INT32_MAX)

-- 
Anton Johansson,
rev.ng Labs Srl.


RE: [PATCH v2] Hexagon (target/hexagon) Additional instructions handled by idef-parser
Posted by Taylor Simpson 12 months ago

> -----Original Message-----
> From: Anton Johansson <anjo@rev.ng>
> Sent: Friday, April 28, 2023 11:25 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: richard.henderson@linaro.org; philmd@linaro.org; ale@rev.ng; Brian Cain
> <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>
> Subject: Re: [PATCH v2] Hexagon (target/hexagon) Additional instructions
> handled by idef-parser
> 
> On 4/26/23 19:32, Taylor Simpson wrote:
> > **** Changes in v2 ****
> > Fix bug in imm_print identified in clang build
> >
> > Currently, idef-parser skips all floating point instructions.
> > However, there are some floating point instructions that can be handled.
> >
> > The following instructions are now parsed
> >      F2_sfimm_p
> >      F2_sfimm_n
> >      F2_dfimm_p
> >      F2_dfimm_n
> >      F2_dfmpyll
> >      F2_dfmpylh
> >
> > To make these instructions work, we fix some bugs in parser-helpers.c
> >      gen_rvalue_extend
> >      gen_cast_op
> >      imm_print
> >
> > Test cases added to tests/tcg/hexagon/fpstuff.c
> >
> > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> > ---
> >   target/hexagon/idef-parser/parser-helpers.h |  2 +-
> >   target/hexagon/idef-parser/parser-helpers.c | 37 ++++++++++----
> >   tests/tcg/hexagon/fpstuff.c                 | 54 +++++++++++++++++++++
> >   target/hexagon/gen_idef_parser_funcs.py     | 10 +++-
> >   4 files changed, 91 insertions(+), 12 deletions(-)
> 
> I'm getting a harness failure on
> 
>      v65_Q6_R_mpy_RR_rnd.c
> 
> I'll take a deeper look at this next week.

I'm seeing that failure too.  Thanks for looking into it.

Taylor

RE: [PATCH v2] Hexagon (target/hexagon) Additional instructions handled by idef-parser
Posted by Taylor Simpson 12 months ago

> -----Original Message-----
> From: Taylor Simpson
> Sent: Friday, April 28, 2023 3:53 PM
> To: anjo@rev.ng; qemu-devel@nongnu.org
> Cc: richard.henderson@linaro.org; philmd@linaro.org; ale@rev.ng; Brian Cain
> <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>
> Subject: RE: [PATCH v2] Hexagon (target/hexagon) Additional instructions
> handled by idef-parser
> 
> 
> 
> > -----Original Message-----
> > From: Anton Johansson <anjo@rev.ng>
> > Sent: Friday, April 28, 2023 11:25 AM
> > To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> > Cc: richard.henderson@linaro.org; philmd@linaro.org; ale@rev.ng; Brian
> Cain
> > <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> > <quic_mathbern@quicinc.com>
> > Subject: Re: [PATCH v2] Hexagon (target/hexagon) Additional instructions
> > handled by idef-parser
> >
> > On 4/26/23 19:32, Taylor Simpson wrote:
> > > **** Changes in v2 ****
> > > Fix bug in imm_print identified in clang build
> > >
> > > Currently, idef-parser skips all floating point instructions.
> > > However, there are some floating point instructions that can be handled.
> > >
> > > The following instructions are now parsed
> > >      F2_sfimm_p
> > >      F2_sfimm_n
> > >      F2_dfimm_p
> > >      F2_dfimm_n
> > >      F2_dfmpyll
> > >      F2_dfmpylh
> > >
> > > To make these instructions work, we fix some bugs in parser-helpers.c
> > >      gen_rvalue_extend
> > >      gen_cast_op
> > >      imm_print
> > >
> > > Test cases added to tests/tcg/hexagon/fpstuff.c
> > >
> > > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> > > ---
> > >   target/hexagon/idef-parser/parser-helpers.h |  2 +-
> > >   target/hexagon/idef-parser/parser-helpers.c | 37 ++++++++++----
> > >   tests/tcg/hexagon/fpstuff.c                 | 54 +++++++++++++++++++++
> > >   target/hexagon/gen_idef_parser_funcs.py     | 10 +++-
> > >   4 files changed, 91 insertions(+), 12 deletions(-)
> >
> > I'm getting a harness failure on
> >
> >      v65_Q6_R_mpy_RR_rnd.c
> >
> > I'll take a deeper look at this next week.
> 
> I'm seeing that failure too.  Thanks for looking into it.

It's this instruction
void emit_M2_dpmpyss_rnd_s0(DisasContext * ctx, Insn * insn, Packet * pkt,
			    TCGv_i32 RdV, TCGv_i32 RsV, TCGv_i32 RtV)
/* {RdV=(fMPY32SS(RsV,RtV)+fCONSTLL(0x80000000))>>32;} */ {
	TCGv_i64 tmp_0 = tcg_temp_new_i64();
	tcg_gen_ext_i32_i64(tmp_0, RsV);
	TCGv_i64 tmp_1 = tcg_temp_new_i64();
	tcg_gen_ext_i32_i64(tmp_1, RtV);
	TCGv_i64 tmp_2 = tcg_temp_new_i64();
	tcg_gen_mul_i64(tmp_2, tmp_0, tmp_1);
	int64_t qemu_tmp_0 = (int64_t) ((int32_t) - 2147483648);
	TCGv_i64 tmp_3 = tcg_temp_new_i64();
	tcg_gen_addi_i64(tmp_3, tmp_2, qemu_tmp_0);
	int64_t qemu_tmp_1 = (int64_t) ((int32_t) 32);
	TCGv_i64 tmp_4 = tcg_temp_new_i64();
	{
		int64_t shift = qemu_tmp_1;
		if (qemu_tmp_1 >= 64) {
			shift = 64 - 1;
		}
		tcg_gen_sari_i64(tmp_4, tmp_3, shift);
	}
	TCGv_i32 tmp_5 = tcg_temp_new_i32();
	tcg_gen_trunc_i64_tl(tmp_5, tmp_4);
	tcg_gen_mov_i32(RdV, tmp_5);
}

The problem is how we handle fCONSTLL(0x80000000).  In macros.h, it's
    #define fCONSTLL(A) A##LL

The parser is treating it as a cast to int64_t.  However,
     0x80000000LL != (int64_t) 0x80000000

I'll change fCONSTLL from a cast to simply changing the bit_width to 64 and signedness to SIGNED.

Stay tuned for v3 of the patch.

Thanks,
Taylor