[PATCH 1/3] Use sub mode to move block for struct parameter

[PATCH 1/3] Use sub mode to move block for struct parameter

[Jiufu Guo]

Hi,

This patch checks an assignment to see if the "from" is about parameter,
and if the parameter may passing through registers, then use the register
mode to move sub-blocks for the assignment.

Bootstraped and regtested on ppc{,le} and x86_64.
Is this ok for trunk?

BR,
Jeff (Jiufu)

gcc/ChangeLog:

	* expr.cc (move_sub_blocks): New function.
	(expand_assignment): Call move_sub_blocks for assigning from parameter.

---
 gcc/expr.cc | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/gcc/expr.cc b/gcc/expr.cc
index d9407432ea5..201fee6fd9a 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -5559,6 +5559,57 @@ mem_ref_refers_to_non_mem_p (tree ref)
   return non_mem_decl_p (base);
 }
 
+/* Sub routine of expand_assignment, invoked when assigning from a
+   parameter or assigning to a return val on struct type which may
+   be passed through registers.  The mode of register is used to
+   move the content for the assignment.
+
+   This routine generates code for expression FROM which is BLKmode,
+   and move the generated content to TO_RTX by su-blocks in SUB_MODE.  */
+
+static void
+move_sub_blocks (rtx to_rtx, tree from, machine_mode sub_mode, bool nontemporal)
+{
+  HOST_WIDE_INT size, sub_size;
+  int len;
+
+  gcc_assert (MEM_P (to_rtx));
+
+  size = MEM_SIZE (to_rtx).to_constant ();
+  sub_size = GET_MODE_SIZE (sub_mode).to_constant ();
+  len = size / sub_size;
+
+  /* As there are limit registers for passing parameters or return
+     value according target ABI.  It would be not profitable to move
+     through sub-modes, if the size does not follow registers.  */
+  int heurisitic_num = 8;
+  if (!size || (size % sub_size) != 0 || len < 2 || len > heurisitic_num)
+    {
+      push_temp_slots ();
+      rtx result = store_expr (from, to_rtx, 0, nontemporal, false);
+      preserve_temp_slots (result);
+      pop_temp_slots ();
+      return;
+    }
+
+  push_temp_slots ();
+
+  rtx from_rtx;
+  from_rtx = expand_expr (from, NULL_RTX, GET_MODE (to_rtx), EXPAND_NORMAL);
+  for (int i = 0; i < len; i++)
+    {
+      rtx temp = gen_reg_rtx (sub_mode);
+      rtx src = adjust_address (from_rtx, sub_mode, sub_size * i);
+      rtx dest = adjust_address (to_rtx, sub_mode, sub_size * i);
+      emit_move_insn (temp, src);
+      emit_move_insn (dest, temp);
+    }
+
+  preserve_temp_slots (to_rtx);
+  pop_temp_slots ();
+  return;
+}
+
 /* Expand an assignment that stores the value of FROM into TO.  If NONTEMPORAL
    is true, try generating a nontemporal store.  */
 
@@ -6045,6 +6096,25 @@ expand_assignment (tree to, tree from, bool nontemporal)
       return;
     }
 
+  /* If it is assigning from a struct param which may be passed via registers,
+     It would be better to use the register's mode to move sub-blocks for the
+     assignment.  */
+  if (TREE_CODE (from) == PARM_DECL && mode == BLKmode
+      && DECL_INCOMING_RTL (from)
+      && (GET_CODE (DECL_INCOMING_RTL (from)) == PARALLEL
+	  || REG_P (DECL_INCOMING_RTL (from))))
+    {
+      rtx parm = DECL_INCOMING_RTL (from);
+      machine_mode sub_mode;
+      if (REG_P (parm))
+	sub_mode = word_mode;
+      else
+	sub_mode = GET_MODE (XEXP (XVECEXP (parm, 0, 0), 0));
+
+      move_sub_blocks (to_rtx, from, sub_mode, nontemporal);
+      return;
+    }
+
   /* Compute FROM and store the value in the rtx we got.  */
 
   push_temp_slots ();
-- 
2.17.1

[PATCH 2/3] Use sub mode to move block for struct returns

[Jiufu Guo]

Hi,

This patch checks an assignment to see if it is copy block to a return
variable, and if the function return through registers, then use the
register mode to move sub-blocks for the assignment.

Bootstraped and regtested on ppc{,le} and x86_64.
Is this ok for trunk?

BR,
Jeff (Jiufu)

	PR target/65421

gcc/ChangeLog:

	* cfgexpand.cc (expand_used_vars): Mark DECL_USEDBY_RETURN_P for return
	vars.
	* expr.cc (expand_assignment): Call move_sub_blocks for assining to a
	struct return variable.
	* tree-core.h (struct tree_decl_common): Comment DECL_USEDBY_RETURN_P.
	* tree.h (DECL_USEDBY_RETURN_P): New define.

---
 gcc/cfgexpand.cc | 14 ++++++++++++++
 gcc/expr.cc      | 13 +++++++++++++
 gcc/tree-core.h  |  3 ++-
 gcc/tree.h       |  4 ++++
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
index dd29ffffc03..0783cb27a59 100644
--- a/gcc/cfgexpand.cc
+++ b/gcc/cfgexpand.cc
@@ -2158,6 +2158,20 @@ expand_used_vars (bitmap forced_stack_vars)
     frame_phase = off ? align - off : 0;
   }
 
+  /* Mark VARs on returns.  */
+  if (DECL_RESULT (current_function_decl))
+    {
+      edge_iterator ei;
+      edge e;
+      FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds)
+	if (greturn *ret = safe_dyn_cast<greturn *> (last_stmt (e->src)))
+	  {
+	    tree val = gimple_return_retval (ret);
+	    if (val && VAR_P (val))
+	      DECL_USEDBY_RETURN_P (val) = 1;
+	  }
+    }
+
   /* Set TREE_USED on all variables in the local_decls.  */
   FOR_EACH_LOCAL_DECL (cfun, i, var)
     TREE_USED (var) = 1;
diff --git a/gcc/expr.cc b/gcc/expr.cc
index 201fee6fd9a..9be75d6733f 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -6115,6 +6115,19 @@ expand_assignment (tree to, tree from, bool nontemporal)
       return;
     }
 
+  /* If it is assigning to a struct var which will be returned, and the
+   function is returning via registers, it would be better to use the
+   register's mode to move sub-blocks for the assignment.  */
+  if (VAR_P (to) && DECL_USEDBY_RETURN_P (to) && mode == BLKmode
+      && TREE_CODE (from) != CONSTRUCTOR
+      && GET_CODE (DECL_RTL (DECL_RESULT (current_function_decl))) == PARALLEL)
+    {
+      rtx ret = DECL_RTL (DECL_RESULT (current_function_decl));
+      machine_mode sub_mode = GET_MODE (XEXP (XVECEXP (ret, 0, 0), 0));
+      move_sub_blocks (to_rtx, from, sub_mode, nontemporal);
+      return;
+    }
+
   /* Compute FROM and store the value in the rtx we got.  */
 
   push_temp_slots ();
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index e146b133dbd..de4acca9ba8 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -1808,7 +1808,8 @@ struct GTY(()) tree_decl_common {
      In VAR_DECL, PARM_DECL and RESULT_DECL, this is
      DECL_HAS_VALUE_EXPR_P.  */
   unsigned decl_flag_2 : 1;
-  /* In FIELD_DECL, this is DECL_PADDING_P.  */
+  /* In FIELD_DECL, this is DECL_PADDING_P
+     In VAR_DECL, this is DECL_USEDBY_RETURN_P.  */
   unsigned decl_flag_3 : 1;
   /* Logically, these two would go in a theoretical base shared by var and
      parm decl. */
diff --git a/gcc/tree.h b/gcc/tree.h
index 4a19de1c94d..b4fbf226ffc 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -3007,6 +3007,10 @@ extern void decl_value_expr_insert (tree, tree);
 #define DECL_PADDING_P(NODE) \
   (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_3)
 
+/* Used in a VAR_DECL to indicate that it is used by a return stmt.  */
+#define DECL_USEDBY_RETURN_P(NODE) \
+  (VAR_DECL_CHECK (NODE)->decl_common.decl_flag_3)
+
 /* Used in a FIELD_DECL to indicate whether this field is not a flexible
    array member. This is only valid for the last array type field of a
    structure.  */
-- 
2.17.1

[PATCH 3/3] Testcases for move sub blocks on param and ret

[Jiufu Guo]

Hi,

This patch is just add test cases, and tested on ppc64{,le}. 

With previous patches on this serial passed, Bootstrap and regtest
passed on ppc64{,le} and x86_64.

Is this ok for trunk?

BR,
Jeff (Jiufu)

	PR target/65421

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pr65421-1.c: New test.
	* gcc.target/powerpc/pr65421.c: New test.

---
 gcc/testsuite/gcc.target/powerpc/pr65421-1.c | 25 ++++++++++++++++++++
 gcc/testsuite/gcc.target/powerpc/pr65421.c   | 22 +++++++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr65421-1.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr65421.c

diff --git a/gcc/testsuite/gcc.target/powerpc/pr65421-1.c b/gcc/testsuite/gcc.target/powerpc/pr65421-1.c
new file mode 100644
index 00000000000..a5ea675008e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr65421-1.c
@@ -0,0 +1,25 @@
+/* PR target/65421 */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-require-effective-target powerpc_elfv2 } */
+
+typedef struct SA
+{
+  double a[3];
+  long l;
+} A;
+
+/* 2 vec load, 2 vec store.  */
+A ret_arg_pt (A *a){return *a;}
+
+/* 4 std */
+A ret_arg (A a) {return a;}
+
+/* 4 std */
+void st_arg (A a, A *p) {*p = a;}
+
+/* { dg-final { scan-assembler-times {\mlxvd2x\M|\mlxv\M|\mlvx\M} 2 } } */
+/* { dg-final { scan-assembler-times {\mstxvd2x\M|\mstxv\M|\mstvx\M} 2 } } */
+/* { dg-final { scan-assembler-times {\mstd\M} 8 } } */
+
+
diff --git a/gcc/testsuite/gcc.target/powerpc/pr65421.c b/gcc/testsuite/gcc.target/powerpc/pr65421.c
new file mode 100644
index 00000000000..27f69e24e29
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr65421.c
@@ -0,0 +1,22 @@
+/* PR target/65421 */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-require-effective-target powerpc_elfv2 } */
+
+typedef struct SA
+{
+  double a[3];
+} A;
+
+/* 3 lfd */
+A ret_arg_pt (A *a){return *a;}
+
+/* blr */
+A ret_arg (A a) {return a;}
+
+/* 3 stfd */
+void st_arg (A a, A *p) {*p = a;}
+
+/* { dg-final { scan-assembler-times {\mlfd\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mstfd\M} 3 } } */
+/* { dg-final { scan-assembler-times {(?n)^\s+[a-z]} 9 } } */
-- 
2.17.1

Re: [PATCH 1/3] Use sub mode to move block for struct parameter

[Jiufu Guo]

Hi,

Jiufu Guo <guojiufu@linux.ibm.com> writes:

> Hi,
>
> This patch checks an assignment to see if the "from" is about parameter,
> and if the parameter may passing through registers, then use the register
> mode to move sub-blocks for the assignment.
>
> Bootstraped and regtested on ppc{,le} and x86_64.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu)
>
> gcc/ChangeLog:
>
> 	* expr.cc (move_sub_blocks): New function.
> 	(expand_assignment): Call move_sub_blocks for assigning from parameter.
>
> ---
>  gcc/expr.cc | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index d9407432ea5..201fee6fd9a 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -5559,6 +5559,57 @@ mem_ref_refers_to_non_mem_p (tree ref)
>    return non_mem_decl_p (base);
>  }
>  
> +/* Sub routine of expand_assignment, invoked when assigning from a
> +   parameter or assigning to a return val on struct type which may
> +   be passed through registers.  The mode of register is used to
> +   move the content for the assignment.
> +
> +   This routine generates code for expression FROM which is BLKmode,
> +   and move the generated content to TO_RTX by su-blocks in SUB_MODE.  */
> +
> +static void
> +move_sub_blocks (rtx to_rtx, tree from, machine_mode sub_mode, bool nontemporal)
> +{
> +  HOST_WIDE_INT size, sub_size;
> +  int len;
> +
> +  gcc_assert (MEM_P (to_rtx));
> +
> +  size = MEM_SIZE (to_rtx).to_constant ();
> +  sub_size = GET_MODE_SIZE (sub_mode).to_constant ();
> +  len = size / sub_size;
> +
> +  /* As there are limit registers for passing parameters or return
> +     value according target ABI.  It would be not profitable to move
> +     through sub-modes, if the size does not follow registers.  */
> +  int heurisitic_num = 8;
> +  if (!size || (size % sub_size) != 0 || len < 2 || len > heurisitic_num)
> +    {
I think that when each parameter is initialized and handled during
setup, we already figured out how it is passed by registers or stack (or
mixed).  And we could use this 'accurate' information and not need the
arbitray number here.

BR,
Jeff (Jiufu)
> +      push_temp_slots ();
> +      rtx result = store_expr (from, to_rtx, 0, nontemporal, false);
> +      preserve_temp_slots (result);
> +      pop_temp_slots ();
> +      return;
> +    }
> +
> +  push_temp_slots ();
> +
> +  rtx from_rtx;
> +  from_rtx = expand_expr (from, NULL_RTX, GET_MODE (to_rtx), EXPAND_NORMAL);
> +  for (int i = 0; i < len; i++)
> +    {
> +      rtx temp = gen_reg_rtx (sub_mode);
> +      rtx src = adjust_address (from_rtx, sub_mode, sub_size * i);
> +      rtx dest = adjust_address (to_rtx, sub_mode, sub_size * i);
> +      emit_move_insn (temp, src);
> +      emit_move_insn (dest, temp);
> +    }
> +
> +  preserve_temp_slots (to_rtx);
> +  pop_temp_slots ();
> +  return;
> +}
> +
>  /* Expand an assignment that stores the value of FROM into TO.  If NONTEMPORAL
>     is true, try generating a nontemporal store.  */
>  
> @@ -6045,6 +6096,25 @@ expand_assignment (tree to, tree from, bool nontemporal)
>        return;
>      }
>  
> +  /* If it is assigning from a struct param which may be passed via registers,
> +     It would be better to use the register's mode to move sub-blocks for the
> +     assignment.  */
> +  if (TREE_CODE (from) == PARM_DECL && mode == BLKmode
> +      && DECL_INCOMING_RTL (from)
> +      && (GET_CODE (DECL_INCOMING_RTL (from)) == PARALLEL
> +	  || REG_P (DECL_INCOMING_RTL (from))))
> +    {
> +      rtx parm = DECL_INCOMING_RTL (from);
> +      machine_mode sub_mode;
> +      if (REG_P (parm))
> +	sub_mode = word_mode;
> +      else
> +	sub_mode = GET_MODE (XEXP (XVECEXP (parm, 0, 0), 0));
> +
> +      move_sub_blocks (to_rtx, from, sub_mode, nontemporal);
> +      return;
> +    }
> +
>    /* Compute FROM and store the value in the rtx we got.  */
>  
>    push_temp_slots ();

Re: [PATCH 3/3] Testcases for move sub blocks on param and ret

[Segher Boessenkool]

Hi!

Some comments on the testcases:

On Tue, Nov 29, 2022 at 09:45:07PM +0800, Jiufu Guo wrote:
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr65421-1.c
> @@ -0,0 +1,25 @@
> +/* PR target/65421 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +/* { dg-require-effective-target powerpc_elfv2 } */
> +
> +typedef struct SA
> +{
> +  double a[3];
> +  long l;
> +} A;
> +
> +/* 2 vec load, 2 vec store.  */
> +A ret_arg_pt (A *a){return *a;}
> +
> +/* 4 std */
> +A ret_arg (A a) {return a;}
> +
> +/* 4 std */
> +void st_arg (A a, A *p) {*p = a;}
> +
> +/* { dg-final { scan-assembler-times {\mlxvd2x\M|\mlxv\M|\mlvx\M} 2 } } */
> +/* { dg-final { scan-assembler-times {\mstxvd2x\M|\mstxv\M|\mstvx\M} 2 } } */
> +/* { dg-final { scan-assembler-times {\mstd\M} 8 } } */

You need at least ISA 2.06 (power7) to have {l,st}xvd2x, and elfv2 does
not guarantee that.  Have you tested on something old as well (say 970)
to see if the lvx/stvx is generated as expected?  For that you need to
have AltiVec enabled as well, so dg-require that?

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr65421.c
> @@ -0,0 +1,22 @@
> +/* PR target/65421 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +/* { dg-require-effective-target powerpc_elfv2 } */
> +
> +typedef struct SA
> +{
> +  double a[3];
> +} A;
> +
> +/* 3 lfd */
> +A ret_arg_pt (A *a){return *a;}
> +
> +/* blr */
> +A ret_arg (A a) {return a;}
> +
> +/* 3 stfd */
> +void st_arg (A a, A *p) {*p = a;}
> +
> +/* { dg-final { scan-assembler-times {\mlfd\M} 3 } } */
> +/* { dg-final { scan-assembler-times {\mstfd\M} 3 } } */
> +/* { dg-final { scan-assembler-times {(?n)^\s+[a-z]} 9 } } */

Comment that last one?  Just something as simple as "count insns" is
enough :-)


Segher

Re: [PATCH 3/3] Testcases for move sub blocks on param and ret

[Jiufu Guo]

Hi Segher,

Thanks for your comments!

Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hi!
>
> Some comments on the testcases:
>
> On Tue, Nov 29, 2022 at 09:45:07PM +0800, Jiufu Guo wrote:
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr65421-1.c
>> @@ -0,0 +1,25 @@
>> +/* PR target/65421 */
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2" } */
>> +/* { dg-require-effective-target powerpc_elfv2 } */
>> +
>> +typedef struct SA
>> +{
>> +  double a[3];
>> +  long l;
>> +} A;
>> +
>> +/* 2 vec load, 2 vec store.  */
>> +A ret_arg_pt (A *a){return *a;}
>> +
>> +/* 4 std */
>> +A ret_arg (A a) {return a;}
>> +
>> +/* 4 std */
>> +void st_arg (A a, A *p) {*p = a;}
>> +
>> +/* { dg-final { scan-assembler-times {\mlxvd2x\M|\mlxv\M|\mlvx\M} 2 } } */
>> +/* { dg-final { scan-assembler-times {\mstxvd2x\M|\mstxv\M|\mstvx\M} 2 } } */
>> +/* { dg-final { scan-assembler-times {\mstd\M} 8 } } */
>
> You need at least ISA 2.06 (power7) to have {l,st}xvd2x, and elfv2 does
> not guarantee that.  Have you tested on something old as well (say 970)
> to see if the lvx/stvx is generated as expected?  For that you need to
> have AltiVec enabled as well, so dg-require that?

Thanks a lot for point out this! 
Compiling this case on power7 manually, it does not generate these vector
load/store insns.  To check those insns, power7 is needed.

>
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr65421.c
>> @@ -0,0 +1,22 @@
>> +/* PR target/65421 */
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2" } */
>> +/* { dg-require-effective-target powerpc_elfv2 } */
>> +
>> +typedef struct SA
>> +{
>> +  double a[3];
>> +} A;
>> +
>> +/* 3 lfd */
>> +A ret_arg_pt (A *a){return *a;}
>> +
>> +/* blr */
>> +A ret_arg (A a) {return a;}
>> +
>> +/* 3 stfd */
>> +void st_arg (A a, A *p) {*p = a;}
>> +
>> +/* { dg-final { scan-assembler-times {\mlfd\M} 3 } } */
>> +/* { dg-final { scan-assembler-times {\mstfd\M} 3 } } */
>> +/* { dg-final { scan-assembler-times {(?n)^\s+[a-z]} 9 } } */
>
> Comment that last one?  Just something as simple as "count insns" is
> enough :-)
Sure, thanks!

BR,
Jeff (Jiufu)

>
>
> Segher