[PATCH v2 0/3] Hexagon (target/hexagon) Only pass env to generated helper when needed

[PATCH v2 0/3] Hexagon (target/hexagon) Only pass env to generated helper when needed

[Taylor Simpson]

Currently, we pass env to every generated helper.  When the semantics of
the instruction only depend on the arguments, this is unnecessary and
adds extra overhead to the helper call.

**** Changes in v2 ****
- Separate patches to pass P0 and SP explicitly to helpers that need it
- Add the TCG_CALL_NO_RWG_SE flag to any non-HVX helpers that
  don't get ptr to env

Taylor Simpson (3):
  Hexagon (target/hexagon) Pass P0 explicitly to helpers that need it
  Hexagon (target/hexagon) Pass SP explicitly to helpers that need it
  Hexagon (target/hexagon) Only pass env to generated helper when needed

 target/hexagon/gen_tcg.h            |  3 ++
 target/hexagon/macros.h             |  4 +--
 target/hexagon/attribs_def.h.inc    |  1 +
 target/hexagon/gen_helper_protos.py | 10 ++++++-
 target/hexagon/hex_common.py        | 44 +++++++++++++++++++++++++----
 5 files changed, 54 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] Hexagon (target/hexagon) Pass P0 explicitly to helpers that need it

[Taylor Simpson]

Rather than reading P0 from the env, pass it explicitly

Signed-off-by: Taylor Simpson <ltaylorsimpson@gmail.com>
---
 target/hexagon/macros.h      |  2 +-
 target/hexagon/hex_common.py | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 1376d6ccc1..9c700ce8ef 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -358,7 +358,7 @@ static inline TCGv gen_read_ireg(TCGv result, TCGv val, int shift)
 #endif
 #define fREAD_PC() (PC)
 
-#define fREAD_P0() (env->pred[0])
+#define fREAD_P0() (P0)
 
 #define fCHECK_PCALIGN(A)
 
diff --git a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py
index 195620c7ec..2dbd0ea737 100755
--- a/target/hexagon/hex_common.py
+++ b/target/hexagon/hex_common.py
@@ -197,6 +197,10 @@ def get_tagimms():
     return dict(zip(tags, list(map(compute_tag_immediates, tags))))
 
 
+def need_p0(tag):
+    return "A_IMPLICIT_READS_P0" in attribdict[tag]
+
+
 def need_slot(tag):
     if (
         "A_CVI_SCATTER" not in attribdict[tag]
@@ -1118,6 +1122,12 @@ def helper_args(tag, regs, imms):
             "tcg_constant_tl(ctx->next_PC)",
             "target_ulong next_PC"
         ))
+    if need_p0(tag):
+        args.append(HelperArg(
+            "i32",
+            "hex_pred[0]",
+            "uint32_t P0"
+        ))
     if need_slot(tag):
         args.append(HelperArg(
             "i32",
-- 
2.34.1

[PATCH v2 2/3] Hexagon (target/hexagon) Pass SP explicitly to helpers that need it

[Taylor Simpson]

Rather than reading SP from the env, pass it explicitly

Signed-off-by: Taylor Simpson <ltaylorsimpson@gmail.com>
---
 target/hexagon/macros.h          |  2 +-
 target/hexagon/attribs_def.h.inc |  1 +
 target/hexagon/hex_common.py     | 11 +++++++++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 9c700ce8ef..4b8665a9da 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -343,7 +343,7 @@ static inline TCGv gen_read_ireg(TCGv result, TCGv val, int shift)
 
 #define fREAD_LR() (env->gpr[HEX_REG_LR])
 
-#define fREAD_SP() (env->gpr[HEX_REG_SP])
+#define fREAD_SP() (SP)
 #define fREAD_LC0 (env->gpr[HEX_REG_LC0])
 #define fREAD_LC1 (env->gpr[HEX_REG_LC1])
 #define fREAD_SA0 (env->gpr[HEX_REG_SA0])
diff --git a/target/hexagon/attribs_def.h.inc b/target/hexagon/attribs_def.h.inc
index 87942d46f4..8949ee09cf 100644
--- a/target/hexagon/attribs_def.h.inc
+++ b/target/hexagon/attribs_def.h.inc
@@ -117,6 +117,7 @@ DEF_ATTRIB(IMPLICIT_READS_P1, "Reads the P1 register", "", "")
 DEF_ATTRIB(IMPLICIT_READS_P2, "Reads the P2 register", "", "")
 DEF_ATTRIB(IMPLICIT_READS_P3, "Reads the P3 register", "", "")
 DEF_ATTRIB(IMPLICIT_WRITES_USR, "May write USR", "", "")
+DEF_ATTRIB(IMPLICIT_READS_SP, "Reads the SP register", "", "")
 DEF_ATTRIB(COMMUTES, "The operation is communitive", "", "")
 DEF_ATTRIB(DEALLOCRET, "dealloc_return", "", "")
 DEF_ATTRIB(DEALLOCFRAME, "deallocframe", "", "")
diff --git a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py
index 2dbd0ea737..eb28aa98fe 100755
--- a/target/hexagon/hex_common.py
+++ b/target/hexagon/hex_common.py
@@ -101,6 +101,7 @@ def calculate_attribs():
     add_qemu_macro_attrib('fLSBNEW1', 'A_IMPLICIT_READS_P1')
     add_qemu_macro_attrib('fLSBNEW1NOT', 'A_IMPLICIT_READS_P1')
     add_qemu_macro_attrib('fREAD_P3', 'A_IMPLICIT_READS_P3')
+    add_qemu_macro_attrib('fREAD_SP', 'A_IMPLICIT_READS_SP')
 
     # Recurse down macros, find attributes from sub-macros
     macroValues = list(macros.values())
@@ -201,6 +202,10 @@ def need_p0(tag):
     return "A_IMPLICIT_READS_P0" in attribdict[tag]
 
 
+def need_sp(tag):
+    return "A_IMPLICIT_READS_SP" in attribdict[tag]
+
+
 def need_slot(tag):
     if (
         "A_CVI_SCATTER" not in attribdict[tag]
@@ -1128,6 +1133,12 @@ def helper_args(tag, regs, imms):
             "hex_pred[0]",
             "uint32_t P0"
         ))
+    if need_sp(tag):
+        args.append(HelperArg(
+            "i32",
+            "hex_gpr[HEX_REG_SP]",
+            "uint32_t SP"
+        ))
     if need_slot(tag):
         args.append(HelperArg(
             "i32",
-- 
2.34.1

[PATCH v2 3/3] Hexagon (target/hexagon) Only pass env to generated helper when needed

[Taylor Simpson]

Currently, we pass env to every generated helper.  When the semantics of
the instruction only depend on the arguments, this is unnecessary and
adds extra overhead to the helper call.

We add the TCG_CALL_NO_RWG_SE flag to any non-HVX helpers that don't get
the ptr to env.

The A2_nop and SA1_setin1 instructions end up with no arguments.  This
results in a "old-style function definition" error from the compiler, so
we write overrides for them.

With this change, the number of helpers with env argument is
    idef-parser enabled:    329 total, 23 with env
    idef-parser disabled:   1543 total, 550 with env

Signed-off-by: Taylor Simpson <ltaylorsimpson@gmail.com>
---
 target/hexagon/gen_tcg.h            |  3 +++
 target/hexagon/gen_helper_protos.py | 10 +++++++++-
 target/hexagon/hex_common.py        | 23 ++++++++++++++++++-----
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/target/hexagon/gen_tcg.h b/target/hexagon/gen_tcg.h
index 1c4391b415..e9ac2e3fe0 100644
--- a/target/hexagon/gen_tcg.h
+++ b/target/hexagon/gen_tcg.h
@@ -1369,3 +1369,6 @@
         gen_helper_raise_exception(tcg_env, excp); \
     } while (0)
 #endif
+
+#define fGEN_TCG_A2_nop(SHORTCODE) do { } while (0)
+#define fGEN_TCG_SA1_setin1(SHORTCODE) tcg_gen_movi_tl(RdV, -1)
diff --git a/target/hexagon/gen_helper_protos.py b/target/hexagon/gen_helper_protos.py
index c82b0f54e4..6e98429752 100755
--- a/target/hexagon/gen_helper_protos.py
+++ b/target/hexagon/gen_helper_protos.py
@@ -40,7 +40,15 @@ def gen_helper_prototype(f, tag, tagregs, tagimms):
         declared.append(arg.proto_arg)
 
     arguments = ", ".join(declared)
-    f.write(f"DEF_HELPER_{len(declared) - 1}({tag}, {arguments})\n")
+
+    ## Add the TCG_CALL_NO_RWG_SE flag to helpers that don't take the env
+    ## argument and aren't HVX instructions.  Since HVX instructions take
+    ## pointers to their arguments, they will have side effects.
+    if hex_common.need_env(tag) or hex_common.is_hvx_insn(tag):
+        f.write(f"DEF_HELPER_{len(declared) - 1}({tag}, {arguments})\n")
+    else:
+        f.write(f"DEF_HELPER_FLAGS_{len(declared) - 1}({tag}, "
+                f"TCG_CALL_NO_RWG_SE, {arguments})\n")
 
 
 def main():
diff --git a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py
index eb28aa98fe..ee08195aba 100755
--- a/target/hexagon/hex_common.py
+++ b/target/hexagon/hex_common.py
@@ -206,6 +206,18 @@ def need_sp(tag):
     return "A_IMPLICIT_READS_SP" in attribdict[tag]
 
 
+def is_hvx_insn(tag):
+    return "A_CVI" in attribdict[tag]
+
+
+def need_env(tag):
+    return ("A_STORE" in attribdict[tag] or
+            "A_LOAD" in attribdict[tag] or
+            "A_CVI_GATHER" in attribdict[tag] or
+            "A_CVI_SCATTER" in attribdict[tag] or
+            "A_IMPLICIT_WRITES_USR" in attribdict[tag])
+
+
 def need_slot(tag):
     if (
         "A_CVI_SCATTER" not in attribdict[tag]
@@ -1069,11 +1081,12 @@ def helper_args(tag, regs, imms):
     args = []
 
     ## First argument is the CPU state
-    args.append(HelperArg(
-        "env",
-        "tcg_env",
-        "CPUHexagonState *env"
-    ))
+    if need_env(tag):
+        args.append(HelperArg(
+            "env",
+            "tcg_env",
+            "CPUHexagonState *env"
+        ))
 
     ## For predicated instructions, we pass in the destination register
     if is_predicated(tag):
-- 
2.34.1

Re: [PATCH v2 0/3] Hexagon (target/hexagon) Only pass env to generated helper when needed

[Anton Johansson via]

On 06/02/24, Taylor Simpson wrote:
> Currently, we pass env to every generated helper.  When the semantics of
> the instruction only depend on the arguments, this is unnecessary and
> adds extra overhead to the helper call.
> 
> **** Changes in v2 ****
> - Separate patches to pass P0 and SP explicitly to helpers that need it
> - Add the TCG_CALL_NO_RWG_SE flag to any non-HVX helpers that
>   don't get ptr to env
> 
> Taylor Simpson (3):
>   Hexagon (target/hexagon) Pass P0 explicitly to helpers that need it
>   Hexagon (target/hexagon) Pass SP explicitly to helpers that need it
>   Hexagon (target/hexagon) Only pass env to generated helper when needed
> 
>  target/hexagon/gen_tcg.h            |  3 ++
>  target/hexagon/macros.h             |  4 +--
>  target/hexagon/attribs_def.h.inc    |  1 +
>  target/hexagon/gen_helper_protos.py | 10 ++++++-
>  target/hexagon/hex_common.py        | 44 +++++++++++++++++++++++++----
>  5 files changed, 54 insertions(+), 8 deletions(-)
> 
> -- 
> 2.34.1
> 

Series:
Reviewed-by: Anton Johansson <anjo@rev.ng>
Tested-by: Anton Johansson <anjo@rev.ng>

-- 
Anton Johansson
rev.ng Labs Srl.