[PATCH] rs6000: Convert the vector element register to SImode [PR98914]

[PATCH] rs6000: Convert the vector element register to SImode [PR98914]

[Xionghu Luo]

v[k] will also be expanded to IFN VEC_SET if k is long type when built
with -Og.  -O0 didn't exposed the issue due to v is TREE_ADDRESSABLE,
-O1 and above also didn't capture it because of v[k] is not optimized to
VIEW_CONVERT_EXPR<int[4]>(v)[k_1].
vec_insert defines the element argument type to be signed int by ELFv2
ABI, so convert it to SImode if it wasn't for Power target requirements.

gcc/ChangeLog:

2021-02-03  Xionghu Luo  <luoxhu@linux.ibm.com>

	* config/rs6000/rs6000.c (rs6000_expand_vector_set): Convert
	elt_rtx to SImode if it wasn't.

gcc/testsuite/ChangeLog:

2021-02-03  Xionghu Luo  <luoxhu@linux.ibm.com>

	* gcc.target/powerpc/pr98914.c: New test.
---
 gcc/config/rs6000/rs6000.c                 | 17 ++++++++++-------
 gcc/testsuite/gcc.target/powerpc/pr98914.c | 11 +++++++++++
 2 files changed, 21 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr98914.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index ec068c58aa5..9f7f8da56c6 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -7000,8 +7000,6 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
 
   gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
 
-  gcc_assert (GET_MODE (idx) == E_SImode);
-
   machine_mode inner_mode = GET_MODE (val);
 
   rtx tmp = gen_reg_rtx (GET_MODE (idx));
@@ -7047,8 +7045,6 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
 
   gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
 
-  gcc_assert (GET_MODE (idx) == E_SImode);
-
   machine_mode inner_mode = GET_MODE (val);
   HOST_WIDE_INT mode_mask = GET_MODE_MASK (inner_mode);
 
@@ -7144,7 +7140,7 @@ rs6000_expand_vector_set (rtx target, rtx val, rtx elt_rtx)
   machine_mode mode = GET_MODE (target);
   machine_mode inner_mode = GET_MODE_INNER (mode);
   rtx reg = gen_reg_rtx (mode);
-  rtx mask, mem, x;
+  rtx mask, mem, x, elt_si;
   int width = GET_MODE_SIZE (inner_mode);
   int i;
 
@@ -7154,16 +7150,23 @@ rs6000_expand_vector_set (rtx target, rtx val, rtx elt_rtx)
     {
       if (!CONST_INT_P (elt_rtx))
 	{
+	  /* elt_rtx should be SImode from ELFv2 ABI.  */
+	  elt_si = gen_reg_rtx (E_SImode);
+	  if (GET_MODE (elt_rtx) != E_SImode)
+	    convert_move (elt_si, elt_rtx, 0);
+	  else
+	    elt_si = elt_rtx;
+
 	  /* For V2DI/V2DF, could leverage the P9 version to generate xxpermdi
 	     when elt_rtx is variable.  */
 	  if ((TARGET_P9_VECTOR && TARGET_POWERPC64) || width == 8)
 	    {
-	      rs6000_expand_vector_set_var_p9 (target, val, elt_rtx);
+	      rs6000_expand_vector_set_var_p9 (target, val, elt_si);
 	      return;
 	    }
 	  else if (TARGET_P8_VECTOR && TARGET_DIRECT_MOVE_64BIT)
 	    {
-	      rs6000_expand_vector_set_var_p8 (target, val, elt_rtx);
+	      rs6000_expand_vector_set_var_p8 (target, val, elt_si);
 	      return;
 	    }
 	}
diff --git a/gcc/testsuite/gcc.target/powerpc/pr98914.c b/gcc/testsuite/gcc.target/powerpc/pr98914.c
new file mode 100644
index 00000000000..e4d78e3e6b3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr98914.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-options "-Og -mvsx" } */
+
+vector int
+foo (vector int v)
+{
+  for (long k = 0; k < 1; ++k)
+    v[k] = 0;
+  return v;
+}
-- 
2.25.1

Ping: [PATCH] rs6000: Convert the vector element register to SImode [PR98914]

[Xionghu Luo]

Gentle ping, thanks.


On 2021/2/3 17:01, Xionghu Luo wrote:
> v[k] will also be expanded to IFN VEC_SET if k is long type when built
> with -Og.  -O0 didn't exposed the issue due to v is TREE_ADDRESSABLE,
> -O1 and above also didn't capture it because of v[k] is not optimized to
> VIEW_CONVERT_EXPR<int[4]>(v)[k_1].
> vec_insert defines the element argument type to be signed int by ELFv2
> ABI, so convert it to SImode if it wasn't for Power target requirements.
> 
> gcc/ChangeLog:
> 
> 2021-02-03  Xionghu Luo  <luoxhu@linux.ibm.com>
> 
> 	* config/rs6000/rs6000.c (rs6000_expand_vector_set): Convert
> 	elt_rtx to SImode if it wasn't.
> 
> gcc/testsuite/ChangeLog:
> 
> 2021-02-03  Xionghu Luo  <luoxhu@linux.ibm.com>
> 
> 	* gcc.target/powerpc/pr98914.c: New test.
> ---
>   gcc/config/rs6000/rs6000.c                 | 17 ++++++++++-------
>   gcc/testsuite/gcc.target/powerpc/pr98914.c | 11 +++++++++++
>   2 files changed, 21 insertions(+), 7 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.target/powerpc/pr98914.c
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index ec068c58aa5..9f7f8da56c6 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -7000,8 +7000,6 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
>   
>     gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
>   
> -  gcc_assert (GET_MODE (idx) == E_SImode);
> -
>     machine_mode inner_mode = GET_MODE (val);
>   
>     rtx tmp = gen_reg_rtx (GET_MODE (idx));
> @@ -7047,8 +7045,6 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
>   
>     gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
>   
> -  gcc_assert (GET_MODE (idx) == E_SImode);
> -
>     machine_mode inner_mode = GET_MODE (val);
>     HOST_WIDE_INT mode_mask = GET_MODE_MASK (inner_mode);
>   
> @@ -7144,7 +7140,7 @@ rs6000_expand_vector_set (rtx target, rtx val, rtx elt_rtx)
>     machine_mode mode = GET_MODE (target);
>     machine_mode inner_mode = GET_MODE_INNER (mode);
>     rtx reg = gen_reg_rtx (mode);
> -  rtx mask, mem, x;
> +  rtx mask, mem, x, elt_si;
>     int width = GET_MODE_SIZE (inner_mode);
>     int i;
>   
> @@ -7154,16 +7150,23 @@ rs6000_expand_vector_set (rtx target, rtx val, rtx elt_rtx)
>       {
>         if (!CONST_INT_P (elt_rtx))
>   	{
> +	  /* elt_rtx should be SImode from ELFv2 ABI.  */
> +	  elt_si = gen_reg_rtx (E_SImode);
> +	  if (GET_MODE (elt_rtx) != E_SImode)
> +	    convert_move (elt_si, elt_rtx, 0);
> +	  else
> +	    elt_si = elt_rtx;
> +
>   	  /* For V2DI/V2DF, could leverage the P9 version to generate xxpermdi
>   	     when elt_rtx is variable.  */
>   	  if ((TARGET_P9_VECTOR && TARGET_POWERPC64) || width == 8)
>   	    {
> -	      rs6000_expand_vector_set_var_p9 (target, val, elt_rtx);
> +	      rs6000_expand_vector_set_var_p9 (target, val, elt_si);
>   	      return;
>   	    }
>   	  else if (TARGET_P8_VECTOR && TARGET_DIRECT_MOVE_64BIT)
>   	    {
> -	      rs6000_expand_vector_set_var_p8 (target, val, elt_rtx);
> +	      rs6000_expand_vector_set_var_p8 (target, val, elt_si);
>   	      return;
>   	    }
>   	}
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr98914.c b/gcc/testsuite/gcc.target/powerpc/pr98914.c
> new file mode 100644
> index 00000000000..e4d78e3e6b3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr98914.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* { dg-options "-Og -mvsx" } */
> +
> +vector int
> +foo (vector int v)
> +{
> +  for (long k = 0; k < 1; ++k)
> +    v[k] = 0;
> +  return v;
> +}
> 

-- 
Thanks,
Xionghu

Re: [PATCH] rs6000: Convert the vector element register to SImode [PR98914]

[will schmidt]

On Wed, 2021-02-03 at 03:01 -0600, Xionghu Luo via Gcc-patches wrote:

Hi,

> v[k] will also be expanded to IFN VEC_SET if k is long type when
> built
> with -Og.  -O0 didn't exposed the issue due to v is TREE_ADDRESSABLE,
> -O1 and above also didn't capture it because of v[k] is not optimized
> to
> VIEW_CONVERT_EXPR<int[4]>(v)[k_1].
> vec_insert defines the element argument type to be signed int by
> ELFv2
> ABI, so convert it to SImode if it wasn't for Power target
> requirements.

The intro paragraph seems to start mid sentence.  Did something get cut
off?
The description here is specific to the reported testcase failure. 
This should describe the patch behavior instead.  Something like 
"When
expanding a vector with a variable rtx, the rtx type needs to be SI"
...
(I defer to any other suggestions of better or improved wording).


> 
> gcc/ChangeLog:


Reference "PR target/98914" somewhere in here.


> 
> 2021-02-03  Xionghu Luo  <luoxhu@linux.ibm.com>
> 
> 	* config/rs6000/rs6000.c (rs6000_expand_vector_set): Convert
> 	elt_rtx to SImode if it wasn't.

s/if it wasn't//


> 
> gcc/testsuite/ChangeLog:
> 
> 2021-02-03  Xionghu Luo  <luoxhu@linux.ibm.com>
> 
> 	* gcc.target/powerpc/pr98914.c: New test.
> ---
>  gcc/config/rs6000/rs6000.c                 | 17 ++++++++++-------
>  gcc/testsuite/gcc.target/powerpc/pr98914.c | 11 +++++++++++
>  2 files changed, 21 insertions(+), 7 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr98914.c
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index ec068c58aa5..9f7f8da56c6 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -7000,8 +7000,6 @@ rs6000_expand_vector_set_var_p9 (rtx target,
> rtx val, rtx idx)
> 
>    gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
> 
> -  gcc_assert (GET_MODE (idx) == E_SImode);
> -
>    machine_mode inner_mode = GET_MODE (val);
> 
>    rtx tmp = gen_reg_rtx (GET_MODE (idx));


This needs a changelog blurb.


> @@ -7047,8 +7045,6 @@ rs6000_expand_vector_set_var_p8 (rtx target,
> rtx val, rtx idx)
> 
>    gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
> 
> -  gcc_assert (GET_MODE (idx) == E_SImode);
> -
>    machine_mode inner_mode = GET_MODE (val);
>    HOST_WIDE_INT mode_mask = GET_MODE_MASK (inner_mode);


Same.

> 
> @@ -7144,7 +7140,7 @@ rs6000_expand_vector_set (rtx target, rtx val,
> rtx elt_rtx)
>    machine_mode mode = GET_MODE (target);
>    machine_mode inner_mode = GET_MODE_INNER (mode);
>    rtx reg = gen_reg_rtx (mode);
> -  rtx mask, mem, x;
> +  rtx mask, mem, x, elt_si;
>    int width = GET_MODE_SIZE (inner_mode);
>    int i;
> 
> @@ -7154,16 +7150,23 @@ rs6000_expand_vector_set (rtx target, rtx
> val, rtx elt_rtx)
>      {
>        if (!CONST_INT_P (elt_rtx))
>  	{
> +	  /* elt_rtx should be SImode from ELFv2 ABI.  */
> +	  elt_si = gen_reg_rtx (E_SImode);
> +	  if (GET_MODE (elt_rtx) != E_SImode)
> +	    convert_move (elt_si, elt_rtx, 0);
> +	  else
> +	    elt_si = elt_rtx;
> +

ok.



>  	  /* For V2DI/V2DF, could leverage the P9 version to generate
> xxpermdi
>  	     when elt_rtx is variable.  */
>  	  if ((TARGET_P9_VECTOR && TARGET_POWERPC64) || width == 8)
>  	    {
> -	      rs6000_expand_vector_set_var_p9 (target, val, elt_rtx);
> +	      rs6000_expand_vector_set_var_p9 (target, val, elt_si);
>  	      return;
>  	    }
>  	  else if (TARGET_P8_VECTOR && TARGET_DIRECT_MOVE_64BIT)
>  	    {
> -	      rs6000_expand_vector_set_var_p8 (target, val, elt_rtx);
> +	      rs6000_expand_vector_set_var_p8 (target, val, elt_si);
>  	      return;
>  	    }
>  	}
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr98914.c
> b/gcc/testsuite/gcc.target/powerpc/pr98914.c
> new file mode 100644
> index 00000000000..e4d78e3e6b3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr98914.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* { dg-options "-Og -mvsx" } */
> +
> +vector int
> +foo (vector int v)
> +{
> +  for (long k = 0; k < 1; ++k)
> +    v[k] = 0;
> +  return v;
> +}
ok

thanks
-Will

Re: [PATCH] rs6000: Convert the vector element register to SImode [PR98914]

[Segher Boessenkool]

Hi!

On Thu, Feb 18, 2021 at 10:46:11AM -0600, will schmidt wrote:
> On Wed, 2021-02-03 at 03:01 -0600, Xionghu Luo via Gcc-patches wrote:
> > v[k] will also be expanded to IFN VEC_SET if k is long type when
> > built
> > with -Og.  -O0 didn't exposed the issue due to v is TREE_ADDRESSABLE,
> > -O1 and above also didn't capture it because of v[k] is not optimized
> > to
> > VIEW_CONVERT_EXPR<int[4]>(v)[k_1].
> > vec_insert defines the element argument type to be signed int by
> > ELFv2
> > ABI, so convert it to SImode if it wasn't for Power target
> > requirements.
> 
> The intro paragraph seems to start mid sentence.  Did something get cut
> off?
> The description here is specific to the reported testcase failure. 
> This should describe the patch behavior instead.  Something like 
> "When
> expanding a vector with a variable rtx, the rtx type needs to be SI"
> ...

mode, not type :-)

> > gcc/ChangeLog:
> 
> 
> Reference "PR target/98914" somewhere in here.

Exactly like that, and in *both* changelogs (gcc/ and gcc/testsuite/),
before all entries.

> > --- a/gcc/config/rs6000/rs6000.c
> > +++ b/gcc/config/rs6000/rs6000.c
> > @@ -7000,8 +7000,6 @@ rs6000_expand_vector_set_var_p9 (rtx target,
> > rtx val, rtx idx)
> > 
> >    gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
> > 
> > -  gcc_assert (GET_MODE (idx) == E_SImode);
> > -
> >    machine_mode inner_mode = GET_MODE (val);
> > 
> >    rtx tmp = gen_reg_rtx (GET_MODE (idx));
> 
> 
> This needs a changelog blurb.

Yup...  Just "Remove assert." will do.

> > @@ -7144,7 +7140,7 @@ rs6000_expand_vector_set (rtx target, rtx val,
> > rtx elt_rtx)
> >    machine_mode mode = GET_MODE (target);
> >    machine_mode inner_mode = GET_MODE_INNER (mode);
> >    rtx reg = gen_reg_rtx (mode);
> > -  rtx mask, mem, x;
> > +  rtx mask, mem, x, elt_si;
> >    int width = GET_MODE_SIZE (inner_mode);
> >    int i;
> > 
> > @@ -7154,16 +7150,23 @@ rs6000_expand_vector_set (rtx target, rtx
> > val, rtx elt_rtx)
> >      {
> >        if (!CONST_INT_P (elt_rtx))
> >  	{
> > +	  /* elt_rtx should be SImode from ELFv2 ABI.  */
> > +	  elt_si = gen_reg_rtx (E_SImode);

Declare it only here, not earlier please.

> > +	  if (GET_MODE (elt_rtx) != E_SImode)
> > +	    convert_move (elt_si, elt_rtx, 0);
> > +	  else
> > +	    elt_si = elt_rtx;

Just always do the convert_move?  So:

  rtx elt_si = gen_reg_rtx (SImode); // no need for that E_ stuff
  convert_move (elt_si, elt_rtx, 0);

This code runs at expand time, so we have many later passes that all can
get rid of that extra move no problem.

> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/pr98914.c
> > @@ -0,0 +1,11 @@
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target powerpc_p8vector_ok } */
> > +/* { dg-options "-Og -mvsx" } */

Please use
  -Og -mdejagnu-cpu=power8
instead.  Was this tested on BE configs?  Please make sure you do.

Please fix (also all Will's other comments, thanks as always!) and
repost (after testing again, of course).  Thanks!


Segher

[PATCH v2] rs6000: Convert the vector element register to SImode [PR98914]

[Xionghu Luo]

vec_insert defines the element argument type to be signed int by ELFv2
ABI, When expanding a vector with a variable rtx, convert the rtx type
SImode.

gcc/ChangeLog:

2021-02-24  Xionghu Luo  <luoxhu@linux.ibm.com>

	PR target/98914
	* config/rs6000/rs6000.c (rs6000_expand_vector_set): Convert
	elt_rtx to SImode.
	(rs6000_expand_vector_set_var_p9): Remove assert.
	(rs6000_expand_vector_set_var_p8): Likewise.

gcc/testsuite/ChangeLog:

2021-02-24  Xionghu Luo  <luoxhu@linux.ibm.com>

	* gcc.target/powerpc/pr98914.c: New test.
---
 gcc/config/rs6000/rs6000.c                 | 17 ++++++++++-------
 gcc/testsuite/gcc.target/powerpc/pr98914.c | 11 +++++++++++
 2 files changed, 21 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr98914.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index ec068c58aa5..9f7f8da56c6 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -7000,8 +7000,6 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
 
   gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
 
-  gcc_assert (GET_MODE (idx) == E_SImode);
-
   machine_mode inner_mode = GET_MODE (val);
 
   rtx tmp = gen_reg_rtx (GET_MODE (idx));
@@ -7047,8 +7045,6 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
 
   gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
 
-  gcc_assert (GET_MODE (idx) == E_SImode);
-
   machine_mode inner_mode = GET_MODE (val);
   HOST_WIDE_INT mode_mask = GET_MODE_MASK (inner_mode);
 
@@ -7144,7 +7140,7 @@ rs6000_expand_vector_set (rtx target, rtx val, rtx elt_rtx)
   machine_mode mode = GET_MODE (target);
   machine_mode inner_mode = GET_MODE_INNER (mode);
   rtx reg = gen_reg_rtx (mode);
-  rtx mask, mem, x;
+  rtx mask, mem, x, elt_si;
   int width = GET_MODE_SIZE (inner_mode);
   int i;
 
@@ -7154,16 +7150,23 @@ rs6000_expand_vector_set (rtx target, rtx val, rtx elt_rtx)
     {
       if (!CONST_INT_P (elt_rtx))
 	{
+	  /* elt_rtx should be SImode from ELFv2 ABI.  */
+	  elt_si = gen_reg_rtx (E_SImode);
+	  if (GET_MODE (elt_rtx) != E_SImode)
+	    convert_move (elt_si, elt_rtx, 0);
+	  else
+	    elt_si = elt_rtx;
+
 	  /* For V2DI/V2DF, could leverage the P9 version to generate xxpermdi
 	     when elt_rtx is variable.  */
 	  if ((TARGET_P9_VECTOR && TARGET_POWERPC64) || width == 8)
 	    {
-	      rs6000_expand_vector_set_var_p9 (target, val, elt_rtx);
+	      rs6000_expand_vector_set_var_p9 (target, val, elt_si);
 	      return;
 	    }
 	  else if (TARGET_P8_VECTOR && TARGET_DIRECT_MOVE_64BIT)
 	    {
-	      rs6000_expand_vector_set_var_p8 (target, val, elt_rtx);
+	      rs6000_expand_vector_set_var_p8 (target, val, elt_si);
 	      return;
 	    }
 	}
diff --git a/gcc/testsuite/gcc.target/powerpc/pr98914.c b/gcc/testsuite/gcc.target/powerpc/pr98914.c
new file mode 100644
index 00000000000..e4d78e3e6b3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr98914.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-options "-Og -mvsx" } */
+
+vector int
+foo (vector int v)
+{
+  for (long k = 0; k < 1; ++k)
+    v[k] = 0;
+  return v;
+}
-- 
2.25.1

Re: [PATCH v2] rs6000: Convert the vector element register to SImode [PR98914]

[Segher Boessenkool]

Hi!

On Wed, Feb 24, 2021 at 09:06:24AM +0800, Xionghu Luo wrote:
> vec_insert defines the element argument type to be signed int by ELFv2
> ABI, When expanding a vector with a variable rtx, convert the rtx type
> SImode.

But that is true for the intrinsics, not for all other callers of
rs6000_expand_vector_init.  See
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98914#c2> as well?

So I don't think you do this in the right place.  You can convince me
with good arguments of course :-)


Segher

Re: [PATCH v2] rs6000: Convert the vector element register to SImode [PR98914]

[Xionghu Luo]

On 2021/2/25 00:57, Segher Boessenkool wrote:
> Hi!
> 
> On Wed, Feb 24, 2021 at 09:06:24AM +0800, Xionghu Luo wrote:
>> vec_insert defines the element argument type to be signed int by ELFv2
>> ABI, When expanding a vector with a variable rtx, convert the rtx type
>> SImode.
> 
> But that is true for the intrinsics, not for all other callers of
> rs6000_expand_vector_init.  See
> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98914#c2> as well?
> 
> So I don't think you do this in the right place.  You can convince me
> with good arguments of course :-)

Thanks for pointing out, it seems we should convert the type to DImode in 
rs6000_expand_vector_set_var_p9 and rs6000_expand_vector_set_var_p8
to support both usage?

 
PS: for "vec_insert (i, u, n)" usage when n is long, what should the front-end
do in altivec_resolve_overloaded_builtin to follow the ELFv2 rule?  Currently,
no warning/error message or conversion there, INTEGRAL_TYPE_P range is much larger
than signed int.


gcc/config/rs6000/rs6000-c.c
altivec_resolve_overloaded_builtin
{
...
      if (!INTEGRAL_TYPE_P (TREE_TYPE (arg2)))
	goto bad;
...
}



Updated the back-end patch as below.


0001-rs6000-Convert-the-vector-set-variable-idx-to-DImode.patch


vec_insert defines the element argument type to be signed int by ELFv2
ABI.  When expanding a vector with a variable rtx, convert the rtx type
to DImode to support both intrinsic usage and other callers from
rs6000_expand_vector_init produced by v[k] = val when k is long type.

gcc/ChangeLog:

2021-02-25  Xionghu Luo  <luoxhu@linux.ibm.com>

	PR target/98914
	* config/rs6000/rs6000.c (rs6000_expand_vector_set_var_p9):
	Convert idx to DImode.
	(rs6000_expand_vector_set_var_p8): Likewise.

gcc/testsuite/ChangeLog:

2021-02-25  Xionghu Luo  <luoxhu@linux.ibm.com>

	PR target/98914
	* gcc.target/powerpc/pr98914.c: New test.
---
 gcc/config/rs6000/rs6000.c                 | 33 +++++++++++++---------
 gcc/testsuite/gcc.target/powerpc/pr98914.c | 11 ++++++++
 2 files changed, 30 insertions(+), 14 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr98914.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index ec068c58aa5..48eb91132a9 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -7000,11 +7000,15 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
 
   gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
 
-  gcc_assert (GET_MODE (idx) == E_SImode);
-
   machine_mode inner_mode = GET_MODE (val);
 
-  rtx tmp = gen_reg_rtx (GET_MODE (idx));
+  machine_mode idx_mode = GET_MODE (idx);
+  rtx tmp = gen_reg_rtx (DImode);
+  if (idx_mode != DImode)
+    tmp = convert_modes (DImode, idx_mode, idx, 0);
+  else
+    tmp = idx;
+
   int width = GET_MODE_SIZE (inner_mode);
 
   gcc_assert (width >= 1 && width <= 8);
@@ -7012,9 +7016,7 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
   int shift = exact_log2 (width);
   /* Generate the IDX for permute shift, width is the vector element size.
      idx = idx * width.  */
-  emit_insn (gen_ashlsi3 (tmp, idx, GEN_INT (shift)));
-
-  tmp = convert_modes (DImode, SImode, tmp, 1);
+  emit_insn (gen_ashldi3 (tmp, tmp, GEN_INT (shift)));
 
   /*  lvsr    v1,0,idx.  */
   rtx pcvr = gen_reg_rtx (V16QImode);
@@ -7047,27 +7049,31 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
 
   gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
 
-  gcc_assert (GET_MODE (idx) == E_SImode);
-
   machine_mode inner_mode = GET_MODE (val);
   HOST_WIDE_INT mode_mask = GET_MODE_MASK (inner_mode);
 
-  rtx tmp = gen_reg_rtx (GET_MODE (idx));
   int width = GET_MODE_SIZE (inner_mode);
 
+  machine_mode idx_mode = GET_MODE (idx);
+  rtx tmp = gen_reg_rtx (DImode);
+  if (idx_mode != DImode)
+    tmp = convert_modes (DImode, idx_mode, idx, 0);
+  else
+    tmp = idx;
+
   gcc_assert (width >= 1 && width <= 4);
 
   if (!BYTES_BIG_ENDIAN)
     {
       /*  idx = idx * width.  */
-      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
+      emit_insn (gen_muldi3 (tmp, tmp, GEN_INT (width)));
       /*  idx = idx + 8.  */
-      emit_insn (gen_addsi3 (tmp, tmp, GEN_INT (8)));
+      emit_insn (gen_adddi3 (tmp, tmp, GEN_INT (8)));
     }
   else
     {
-      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
-      emit_insn (gen_subsi3 (tmp, GEN_INT (24 - width), tmp));
+      emit_insn (gen_muldi3 (tmp, idx, GEN_INT (width)));
+      emit_insn (gen_subdi3 (tmp, GEN_INT (24 - width), tmp));
     }
 
   /*  lxv vs33, mask.
@@ -7118,7 +7124,6 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
   emit_insn (gen_rtx_SET (val_v16qi, sub_val));
 
   /*  lvsl    13,0,idx.  */
-  tmp = convert_modes (DImode, SImode, tmp, 1);
   rtx pcv = gen_reg_rtx (V16QImode);
   emit_insn (gen_altivec_lvsl_reg (pcv, tmp));
 
diff --git a/gcc/testsuite/gcc.target/powerpc/pr98914.c b/gcc/testsuite/gcc.target/powerpc/pr98914.c
new file mode 100644
index 00000000000..e4d78e3e6b3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr98914.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-options "-Og -mvsx" } */
+
+vector int
+foo (vector int v)
+{
+  for (long k = 0; k < 1; ++k)
+    v[k] = 0;
+  return v;
+}
-- 
2.25.1

Ping: [PATCH v2] rs6000: Convert the vector element register to SImode [PR98914]

[Xionghu Luo]

On 2021/2/25 14:33, Xionghu Luo via Gcc-patches wrote:
> 
> 
> On 2021/2/25 00:57, Segher Boessenkool wrote:
>> Hi!
>>
>> On Wed, Feb 24, 2021 at 09:06:24AM +0800, Xionghu Luo wrote:
>>> vec_insert defines the element argument type to be signed int by ELFv2
>>> ABI, When expanding a vector with a variable rtx, convert the rtx type
>>> SImode.
>>
>> But that is true for the intrinsics, not for all other callers of
>> rs6000_expand_vector_init.  See
>> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98914#c2> as well?
>>
>> So I don't think you do this in the right place.  You can convince me
>> with good arguments of course :-)
> 
> Thanks for pointing out, it seems we should convert the type to DImode in
> rs6000_expand_vector_set_var_p9 and rs6000_expand_vector_set_var_p8
> to support both usage?
> 
> 
> PS: for "vec_insert (i, u, n)" usage when n is long, what should the front-end
> do in altivec_resolve_overloaded_builtin to follow the ELFv2 rule?  Currently,
> no warning/error message or conversion there, INTEGRAL_TYPE_P range is much larger
> than signed int.

long to int should follow implicit transformation, so no need change here.
Ping the patch, thanks.


BR,
Xionghu

> 
> 
> 
> Updated the back-end patch as below.
> 
> 
> 0001-rs6000-Convert-the-vector-set-variable-idx-to-DImode.patch
> 
> 
> vec_insert defines the element argument type to be signed int by ELFv2
> ABI.  When expanding a vector with a variable rtx, convert the rtx type
> to DImode to support both intrinsic usage and other callers from
> rs6000_expand_vector_init produced by v[k] = val when k is long type.
> 
> gcc/ChangeLog:
> 
> 2021-02-25  Xionghu Luo  <luoxhu@linux.ibm.com>
> 
> 	PR target/98914
> 	* config/rs6000/rs6000.c (rs6000_expand_vector_set_var_p9):
> 	Convert idx to DImode.
> 	(rs6000_expand_vector_set_var_p8): Likewise.
> 
> gcc/testsuite/ChangeLog:
> 
> 2021-02-25  Xionghu Luo  <luoxhu@linux.ibm.com>
> 
> 	PR target/98914
> 	* gcc.target/powerpc/pr98914.c: New test.
> ---
>   gcc/config/rs6000/rs6000.c                 | 33 +++++++++++++---------
>   gcc/testsuite/gcc.target/powerpc/pr98914.c | 11 ++++++++
>   2 files changed, 30 insertions(+), 14 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.target/powerpc/pr98914.c
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index ec068c58aa5..48eb91132a9 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -7000,11 +7000,15 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
> 
>     gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
> 
> -  gcc_assert (GET_MODE (idx) == E_SImode);
> -
>     machine_mode inner_mode = GET_MODE (val);
> 
> -  rtx tmp = gen_reg_rtx (GET_MODE (idx));
> +  machine_mode idx_mode = GET_MODE (idx);
> +  rtx tmp = gen_reg_rtx (DImode);
> +  if (idx_mode != DImode)
> +    tmp = convert_modes (DImode, idx_mode, idx, 0);
> +  else
> +    tmp = idx;
> +
>     int width = GET_MODE_SIZE (inner_mode);
> 
>     gcc_assert (width >= 1 && width <= 8);
> @@ -7012,9 +7016,7 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
>     int shift = exact_log2 (width);
>     /* Generate the IDX for permute shift, width is the vector element size.
>        idx = idx * width.  */
> -  emit_insn (gen_ashlsi3 (tmp, idx, GEN_INT (shift)));
> -
> -  tmp = convert_modes (DImode, SImode, tmp, 1);
> +  emit_insn (gen_ashldi3 (tmp, tmp, GEN_INT (shift)));
> 
>     /*  lvsr    v1,0,idx.  */
>     rtx pcvr = gen_reg_rtx (V16QImode);
> @@ -7047,27 +7049,31 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
> 
>     gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
> 
> -  gcc_assert (GET_MODE (idx) == E_SImode);
> -
>     machine_mode inner_mode = GET_MODE (val);
>     HOST_WIDE_INT mode_mask = GET_MODE_MASK (inner_mode);
> 
> -  rtx tmp = gen_reg_rtx (GET_MODE (idx));
>     int width = GET_MODE_SIZE (inner_mode);
> 
> +  machine_mode idx_mode = GET_MODE (idx);
> +  rtx tmp = gen_reg_rtx (DImode);
> +  if (idx_mode != DImode)
> +    tmp = convert_modes (DImode, idx_mode, idx, 0);
> +  else
> +    tmp = idx;
> +
>     gcc_assert (width >= 1 && width <= 4);
> 
>     if (!BYTES_BIG_ENDIAN)
>       {
>         /*  idx = idx * width.  */
> -      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
> +      emit_insn (gen_muldi3 (tmp, tmp, GEN_INT (width)));
>         /*  idx = idx + 8.  */
> -      emit_insn (gen_addsi3 (tmp, tmp, GEN_INT (8)));
> +      emit_insn (gen_adddi3 (tmp, tmp, GEN_INT (8)));
>       }
>     else
>       {
> -      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
> -      emit_insn (gen_subsi3 (tmp, GEN_INT (24 - width), tmp));
> +      emit_insn (gen_muldi3 (tmp, idx, GEN_INT (width)));
> +      emit_insn (gen_subdi3 (tmp, GEN_INT (24 - width), tmp));
>       }
> 
>     /*  lxv vs33, mask.
> @@ -7118,7 +7124,6 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
>     emit_insn (gen_rtx_SET (val_v16qi, sub_val));
> 
>     /*  lvsl    13,0,idx.  */
> -  tmp = convert_modes (DImode, SImode, tmp, 1);
>     rtx pcv = gen_reg_rtx (V16QImode);
>     emit_insn (gen_altivec_lvsl_reg (pcv, tmp));
> 
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr98914.c b/gcc/testsuite/gcc.target/powerpc/pr98914.c
> new file mode 100644
> index 00000000000..e4d78e3e6b3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr98914.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* { dg-options "-Og -mvsx" } */
> +
> +vector int
> +foo (vector int v)
> +{
> +  for (long k = 0; k < 1; ++k)
> +    v[k] = 0;
> +  return v;
> +}
> 

-- 
Thanks,
Xionghu

Ping^2: [PATCH v2] rs6000: Convert the vector element register to SImode [PR98914]

[Xionghu Luo]

[-- Attachment #1: Type: text/plain, Size: 5977 bytes --]

Ping^2 for stage 4 P1 issue and attached the patch, Thanks!



On 2021/3/3 09:12, Xionghu Luo via Gcc-patches wrote:
> 
> On 2021/2/25 14:33, Xionghu Luo via Gcc-patches wrote:
>>
>>
>> On 2021/2/25 00:57, Segher Boessenkool wrote:
>>> Hi!
>>>
>>> On Wed, Feb 24, 2021 at 09:06:24AM +0800, Xionghu Luo wrote:
>>>> vec_insert defines the element argument type to be signed int by ELFv2
>>>> ABI, When expanding a vector with a variable rtx, convert the rtx type
>>>> SImode.
>>>
>>> But that is true for the intrinsics, not for all other callers of
>>> rs6000_expand_vector_init.  See
>>> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98914#c2> as well?
>>>
>>> So I don't think you do this in the right place.  You can convince me
>>> with good arguments of course :-)
>>
>> Thanks for pointing out, it seems we should convert the type to DImode in
>> rs6000_expand_vector_set_var_p9 and rs6000_expand_vector_set_var_p8
>> to support both usage?
>>
>>
>> PS: for "vec_insert (i, u, n)" usage when n is long, what should the front-end
>> do in altivec_resolve_overloaded_builtin to follow the ELFv2 rule?  Currently,
>> no warning/error message or conversion there, INTEGRAL_TYPE_P range is much larger
>> than signed int.
> 
> long to int should follow implicit transformation, so no need change here.
> Ping the patch, thanks.
> 
> 
> BR,
> Xionghu
> 
>>
>>
>>
>> Updated the back-end patch as below.
>>
>>
>> 0001-rs6000-Convert-the-vector-set-variable-idx-to-DImode.patch
>>
>>
>> vec_insert defines the element argument type to be signed int by ELFv2
>> ABI.  When expanding a vector with a variable rtx, convert the rtx type
>> to DImode to support both intrinsic usage and other callers from
>> rs6000_expand_vector_init produced by v[k] = val when k is long type.
>>
>> gcc/ChangeLog:
>>
>> 2021-02-25  Xionghu Luo  <luoxhu@linux.ibm.com>
>>
>> 	PR target/98914
>> 	* config/rs6000/rs6000.c (rs6000_expand_vector_set_var_p9):
>> 	Convert idx to DImode.
>> 	(rs6000_expand_vector_set_var_p8): Likewise.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2021-02-25  Xionghu Luo  <luoxhu@linux.ibm.com>
>>
>> 	PR target/98914
>> 	* gcc.target/powerpc/pr98914.c: New test.
>> ---
>>    gcc/config/rs6000/rs6000.c                 | 33 +++++++++++++---------
>>    gcc/testsuite/gcc.target/powerpc/pr98914.c | 11 ++++++++
>>    2 files changed, 30 insertions(+), 14 deletions(-)
>>    create mode 100644 gcc/testsuite/gcc.target/powerpc/pr98914.c
>>
>> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
>> index ec068c58aa5..48eb91132a9 100644
>> --- a/gcc/config/rs6000/rs6000.c
>> +++ b/gcc/config/rs6000/rs6000.c
>> @@ -7000,11 +7000,15 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
>>
>>      gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
>>
>> -  gcc_assert (GET_MODE (idx) == E_SImode);
>> -
>>      machine_mode inner_mode = GET_MODE (val);
>>
>> -  rtx tmp = gen_reg_rtx (GET_MODE (idx));
>> +  machine_mode idx_mode = GET_MODE (idx);
>> +  rtx tmp = gen_reg_rtx (DImode);
>> +  if (idx_mode != DImode)
>> +    tmp = convert_modes (DImode, idx_mode, idx, 0);
>> +  else
>> +    tmp = idx;
>> +
>>      int width = GET_MODE_SIZE (inner_mode);
>>
>>      gcc_assert (width >= 1 && width <= 8);
>> @@ -7012,9 +7016,7 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
>>      int shift = exact_log2 (width);
>>      /* Generate the IDX for permute shift, width is the vector element size.
>>         idx = idx * width.  */
>> -  emit_insn (gen_ashlsi3 (tmp, idx, GEN_INT (shift)));
>> -
>> -  tmp = convert_modes (DImode, SImode, tmp, 1);
>> +  emit_insn (gen_ashldi3 (tmp, tmp, GEN_INT (shift)));
>>
>>      /*  lvsr    v1,0,idx.  */
>>      rtx pcvr = gen_reg_rtx (V16QImode);
>> @@ -7047,27 +7049,31 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
>>
>>      gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
>>
>> -  gcc_assert (GET_MODE (idx) == E_SImode);
>> -
>>      machine_mode inner_mode = GET_MODE (val);
>>      HOST_WIDE_INT mode_mask = GET_MODE_MASK (inner_mode);
>>
>> -  rtx tmp = gen_reg_rtx (GET_MODE (idx));
>>      int width = GET_MODE_SIZE (inner_mode);
>>
>> +  machine_mode idx_mode = GET_MODE (idx);
>> +  rtx tmp = gen_reg_rtx (DImode);
>> +  if (idx_mode != DImode)
>> +    tmp = convert_modes (DImode, idx_mode, idx, 0);
>> +  else
>> +    tmp = idx;
>> +
>>      gcc_assert (width >= 1 && width <= 4);
>>
>>      if (!BYTES_BIG_ENDIAN)
>>        {
>>          /*  idx = idx * width.  */
>> -      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
>> +      emit_insn (gen_muldi3 (tmp, tmp, GEN_INT (width)));
>>          /*  idx = idx + 8.  */
>> -      emit_insn (gen_addsi3 (tmp, tmp, GEN_INT (8)));
>> +      emit_insn (gen_adddi3 (tmp, tmp, GEN_INT (8)));
>>        }
>>      else
>>        {
>> -      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
>> -      emit_insn (gen_subsi3 (tmp, GEN_INT (24 - width), tmp));
>> +      emit_insn (gen_muldi3 (tmp, idx, GEN_INT (width)));
>> +      emit_insn (gen_subdi3 (tmp, GEN_INT (24 - width), tmp));
>>        }
>>
>>      /*  lxv vs33, mask.
>> @@ -7118,7 +7124,6 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
>>      emit_insn (gen_rtx_SET (val_v16qi, sub_val));
>>
>>      /*  lvsl    13,0,idx.  */
>> -  tmp = convert_modes (DImode, SImode, tmp, 1);
>>      rtx pcv = gen_reg_rtx (V16QImode);
>>      emit_insn (gen_altivec_lvsl_reg (pcv, tmp));
>>
>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr98914.c b/gcc/testsuite/gcc.target/powerpc/pr98914.c
>> new file mode 100644
>> index 00000000000..e4d78e3e6b3
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr98914.c
>> @@ -0,0 +1,11 @@
>> +/* { dg-do compile } */
>> +/* { dg-require-effective-target powerpc_p8vector_ok } */
>> +/* { dg-options "-Og -mvsx" } */
>> +
>> +vector int
>> +foo (vector int v)
>> +{
>> +  for (long k = 0; k < 1; ++k)
>> +    v[k] = 0;
>> +  return v;
>> +}
>>
> 

-- 
Thanks,
Xionghu

[-- Attachment #2: 0001-rs6000-Convert-the-vector-set-variable-idx-to-DImode.patch --]
[-- Type: text/plain, Size: 4312 bytes --]

From 40688fb162d125f60c064d0b9b80f83dc1a9afd4 Mon Sep 17 00:00:00 2001
From: Xionghu Luo <luoxhu@linux.ibm.com>
Date: Tue, 2 Feb 2021 23:28:04 -0600
Subject: [PATCH] rs6000: Convert the vector set variable idx to DImode
 [PR98914]

vec_insert defines the element argument type to be signed int by ELFv2
ABI.  When expanding a vector with a variable rtx, convert the rtx type
to DImode to support both intrinsic usage and other callers from
rs6000_expand_vector_init produced by v[k] = val when k is long type.

gcc/ChangeLog:

2021-02-25  Xionghu Luo  <luoxhu@linux.ibm.com>

	PR target/98914
	* config/rs6000/rs6000.c (rs6000_expand_vector_set_var_p9):
	Convert idx to DImode.
	(rs6000_expand_vector_set_var_p8): Likewise.

gcc/testsuite/ChangeLog:

2021-02-25  Xionghu Luo  <luoxhu@linux.ibm.com>

	PR target/98914
	* gcc.target/powerpc/pr98914.c: New test.
---
 gcc/config/rs6000/rs6000.c                 | 33 +++++++++++++---------
 gcc/testsuite/gcc.target/powerpc/pr98914.c | 11 ++++++++
 2 files changed, 30 insertions(+), 14 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr98914.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index ec068c58aa5..48eb91132a9 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -7000,11 +7000,15 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
 
   gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
 
-  gcc_assert (GET_MODE (idx) == E_SImode);
-
   machine_mode inner_mode = GET_MODE (val);
 
-  rtx tmp = gen_reg_rtx (GET_MODE (idx));
+  machine_mode idx_mode = GET_MODE (idx);
+  rtx tmp = gen_reg_rtx (DImode);
+  if (idx_mode != DImode)
+    tmp = convert_modes (DImode, idx_mode, idx, 0);
+  else
+    tmp = idx;
+
   int width = GET_MODE_SIZE (inner_mode);
 
   gcc_assert (width >= 1 && width <= 8);
@@ -7012,9 +7016,7 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
   int shift = exact_log2 (width);
   /* Generate the IDX for permute shift, width is the vector element size.
      idx = idx * width.  */
-  emit_insn (gen_ashlsi3 (tmp, idx, GEN_INT (shift)));
-
-  tmp = convert_modes (DImode, SImode, tmp, 1);
+  emit_insn (gen_ashldi3 (tmp, tmp, GEN_INT (shift)));
 
   /*  lvsr    v1,0,idx.  */
   rtx pcvr = gen_reg_rtx (V16QImode);
@@ -7047,27 +7049,31 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
 
   gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
 
-  gcc_assert (GET_MODE (idx) == E_SImode);
-
   machine_mode inner_mode = GET_MODE (val);
   HOST_WIDE_INT mode_mask = GET_MODE_MASK (inner_mode);
 
-  rtx tmp = gen_reg_rtx (GET_MODE (idx));
   int width = GET_MODE_SIZE (inner_mode);
 
+  machine_mode idx_mode = GET_MODE (idx);
+  rtx tmp = gen_reg_rtx (DImode);
+  if (idx_mode != DImode)
+    tmp = convert_modes (DImode, idx_mode, idx, 0);
+  else
+    tmp = idx;
+
   gcc_assert (width >= 1 && width <= 4);
 
   if (!BYTES_BIG_ENDIAN)
     {
       /*  idx = idx * width.  */
-      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
+      emit_insn (gen_muldi3 (tmp, tmp, GEN_INT (width)));
       /*  idx = idx + 8.  */
-      emit_insn (gen_addsi3 (tmp, tmp, GEN_INT (8)));
+      emit_insn (gen_adddi3 (tmp, tmp, GEN_INT (8)));
     }
   else
     {
-      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
-      emit_insn (gen_subsi3 (tmp, GEN_INT (24 - width), tmp));
+      emit_insn (gen_muldi3 (tmp, idx, GEN_INT (width)));
+      emit_insn (gen_subdi3 (tmp, GEN_INT (24 - width), tmp));
     }
 
   /*  lxv vs33, mask.
@@ -7118,7 +7124,6 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
   emit_insn (gen_rtx_SET (val_v16qi, sub_val));
 
   /*  lvsl    13,0,idx.  */
-  tmp = convert_modes (DImode, SImode, tmp, 1);
   rtx pcv = gen_reg_rtx (V16QImode);
   emit_insn (gen_altivec_lvsl_reg (pcv, tmp));
 
diff --git a/gcc/testsuite/gcc.target/powerpc/pr98914.c b/gcc/testsuite/gcc.target/powerpc/pr98914.c
new file mode 100644
index 00000000000..e4d78e3e6b3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr98914.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-options "-Og -mvsx" } */
+
+vector int
+foo (vector int v)
+{
+  for (long k = 0; k < 1; ++k)
+    v[k] = 0;
+  return v;
+}
-- 
2.25.1

Re: Ping^2: [PATCH v2] rs6000: Convert the vector element register to SImode [PR98914]

[Jakub Jelinek]

On Thu, Mar 11, 2021 at 07:57:23AM +0800, Xionghu Luo via Gcc-patches wrote:
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index ec068c58aa5..48eb91132a9 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -7000,11 +7000,15 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
>  
>    gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
>  
> -  gcc_assert (GET_MODE (idx) == E_SImode);
> -
>    machine_mode inner_mode = GET_MODE (val);
>  
> -  rtx tmp = gen_reg_rtx (GET_MODE (idx));
> +  machine_mode idx_mode = GET_MODE (idx);
> +  rtx tmp = gen_reg_rtx (DImode);
> +  if (idx_mode != DImode)
> +    tmp = convert_modes (DImode, idx_mode, idx, 0);
> +  else
> +    tmp = idx;
> +
>    int width = GET_MODE_SIZE (inner_mode);
>  
>    gcc_assert (width >= 1 && width <= 8);
> @@ -7012,9 +7016,7 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
>    int shift = exact_log2 (width);
>    /* Generate the IDX for permute shift, width is the vector element size.
>       idx = idx * width.  */
> -  emit_insn (gen_ashlsi3 (tmp, idx, GEN_INT (shift)));
> -
> -  tmp = convert_modes (DImode, SImode, tmp, 1);
> +  emit_insn (gen_ashldi3 (tmp, tmp, GEN_INT (shift)));

This looks broken.
The gen_reg_rtx (DImode); call result is completely ignored,
so it wastes one pseudo, and I'm not convinced that idx nor tmp
is guaranteed to be usable as output of shift.
So, shouldn't it be instead:
  rtx tmp = gen_reg_rtx (DImode);
  if (idx_mode != DImode)
    idx = convert_modes (DImode, idx_mode, idx, 0);

...
  emit_insn (gen_ashldi3 (tmp, idx, GEN_INT (shift));
?
Also, dunno what is less and what is more expensive on
rs6000, whether convert_modes with unsigned = 0 or 1.
I think rs6000 only supports very narrow vectors, so even for
say QImode idx anything with MSB set will be out of out of bounds and UB,
so you can sign or zero extend, whatever is more efficient.

> +  machine_mode idx_mode = GET_MODE (idx);
> +  rtx tmp = gen_reg_rtx (DImode);
> +  if (idx_mode != DImode)
> +    tmp = convert_modes (DImode, idx_mode, idx, 0);
> +  else
> +    tmp = idx;
> +
>    gcc_assert (width >= 1 && width <= 4);
>  
>    if (!BYTES_BIG_ENDIAN)
>      {
>        /*  idx = idx * width.  */
> -      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
> +      emit_insn (gen_muldi3 (tmp, tmp, GEN_INT (width)));
>        /*  idx = idx + 8.  */
> -      emit_insn (gen_addsi3 (tmp, tmp, GEN_INT (8)));
> +      emit_insn (gen_adddi3 (tmp, tmp, GEN_INT (8)));
>      }
>    else
>      {
> -      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
> -      emit_insn (gen_subsi3 (tmp, GEN_INT (24 - width), tmp));
> +      emit_insn (gen_muldi3 (tmp, idx, GEN_INT (width)));
> +      emit_insn (gen_subdi3 (tmp, GEN_INT (24 - width), tmp));

Ditto.  But the above was even inconsistent, sometimes it
used tmp (for LE) and otherwise idx in whatever mode it has.

	Jakub

Re: Ping^2: [PATCH v2] rs6000: Convert the vector element register to SImode [PR98914]

[Segher Boessenkool]

Hi!

On Tue, Mar 16, 2021 at 07:57:17PM +0100, Jakub Jelinek wrote:
> On Thu, Mar 11, 2021 at 07:57:23AM +0800, Xionghu Luo via Gcc-patches wrote:
> > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> > index ec068c58aa5..48eb91132a9 100644
> > --- a/gcc/config/rs6000/rs6000.c
> > +++ b/gcc/config/rs6000/rs6000.c
> > @@ -7000,11 +7000,15 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
> >  
> >    gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
> >  
> > -  gcc_assert (GET_MODE (idx) == E_SImode);
> > -
> >    machine_mode inner_mode = GET_MODE (val);
> >  
> > -  rtx tmp = gen_reg_rtx (GET_MODE (idx));
> > +  machine_mode idx_mode = GET_MODE (idx);
> > +  rtx tmp = gen_reg_rtx (DImode);
> > +  if (idx_mode != DImode)
> > +    tmp = convert_modes (DImode, idx_mode, idx, 0);
> > +  else
> > +    tmp = idx;
> > +
> >    int width = GET_MODE_SIZE (inner_mode);
> >  
> >    gcc_assert (width >= 1 && width <= 8);
> > @@ -7012,9 +7016,7 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
> >    int shift = exact_log2 (width);
> >    /* Generate the IDX for permute shift, width is the vector element size.
> >       idx = idx * width.  */
> > -  emit_insn (gen_ashlsi3 (tmp, idx, GEN_INT (shift)));
> > -
> > -  tmp = convert_modes (DImode, SImode, tmp, 1);
> > +  emit_insn (gen_ashldi3 (tmp, tmp, GEN_INT (shift)));
> 
> This looks broken.

Yup.

> The gen_reg_rtx (DImode); call result is completely ignored,
> so it wastes one pseudo, and I'm not convinced that idx nor tmp
> is guaranteed to be usable as output of shift.
> So, shouldn't it be instead:
>   rtx tmp = gen_reg_rtx (DImode);
>   if (idx_mode != DImode)
>     idx = convert_modes (DImode, idx_mode, idx, 0);
> 
> ...
>   emit_insn (gen_ashldi3 (tmp, idx, GEN_INT (shift));
> ?

Yeah, something like that.

Just
  idx = convert_modes (DImode, idx_mode, idx, 1);
...
  rtx tmp = gen_reg_rtx (DImode);
  emit_insn (gen_ashldi3 (tmp, idx, GEN_INT (shift));
works just as well.

> Also, dunno what is less and what is more expensive on
> rs6000, whether convert_modes with unsigned = 0 or 1.

Unsigned is never more expensive.  Sometimes it is cheaper.  It depends
more on the situation what is best.  Just look at the generated code?

> I think rs6000 only supports very narrow vectors, so even for

The only hardware vectors are 128 bits.  There are modes for two vectors
concatenated as well, but that is just for RTL, you should not have
insns making such a pair.

> say QImode idx anything with MSB set will be out of out of bounds and UB,
> so you can sign or zero extend, whatever is more efficient.

Yup.

> > +  machine_mode idx_mode = GET_MODE (idx);
> > +  rtx tmp = gen_reg_rtx (DImode);
> > +  if (idx_mode != DImode)
> > +    tmp = convert_modes (DImode, idx_mode, idx, 0);
> > +  else
> > +    tmp = idx;
> > +
> >    gcc_assert (width >= 1 && width <= 4);
> >  
> >    if (!BYTES_BIG_ENDIAN)
> >      {
> >        /*  idx = idx * width.  */
> > -      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
> > +      emit_insn (gen_muldi3 (tmp, tmp, GEN_INT (width)));
> >        /*  idx = idx + 8.  */
> > -      emit_insn (gen_addsi3 (tmp, tmp, GEN_INT (8)));
> > +      emit_insn (gen_adddi3 (tmp, tmp, GEN_INT (8)));
> >      }
> >    else
> >      {
> > -      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
> > -      emit_insn (gen_subsi3 (tmp, GEN_INT (24 - width), tmp));
> > +      emit_insn (gen_muldi3 (tmp, idx, GEN_INT (width)));
> > +      emit_insn (gen_subdi3 (tmp, GEN_INT (24 - width), tmp));
> 
> Ditto.  But the above was even inconsistent, sometimes it
> used tmp (for LE) and otherwise idx in whatever mode it has.

Thanks for the review Jakub, I kinda dropped this :-(


Segher

Re: Ping^2: [PATCH v2] rs6000: Convert the vector element register to SImode [PR98914]

[Xionghu Luo]

[-- Attachment #1: Type: text/plain, Size: 3848 bytes --]

Thanks Jakub & Segher,

On 2021/3/17 06:47, Segher Boessenkool wrote:
> Hi!
> 
> On Tue, Mar 16, 2021 at 07:57:17PM +0100, Jakub Jelinek wrote:
>> On Thu, Mar 11, 2021 at 07:57:23AM +0800, Xionghu Luo via Gcc-patches wrote:
>>> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
>>> index ec068c58aa5..48eb91132a9 100644
>>> --- a/gcc/config/rs6000/rs6000.c
>>> +++ b/gcc/config/rs6000/rs6000.c
>>> @@ -7000,11 +7000,15 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
>>>   
>>>     gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
>>>   
>>> -  gcc_assert (GET_MODE (idx) == E_SImode);
>>> -
>>>     machine_mode inner_mode = GET_MODE (val);
>>>   
>>> -  rtx tmp = gen_reg_rtx (GET_MODE (idx));
>>> +  machine_mode idx_mode = GET_MODE (idx);
>>> +  rtx tmp = gen_reg_rtx (DImode);
>>> +  if (idx_mode != DImode)
>>> +    tmp = convert_modes (DImode, idx_mode, idx, 0);
>>> +  else
>>> +    tmp = idx;
>>> +
>>>     int width = GET_MODE_SIZE (inner_mode);
>>>   
>>>     gcc_assert (width >= 1 && width <= 8);
>>> @@ -7012,9 +7016,7 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
>>>     int shift = exact_log2 (width);
>>>     /* Generate the IDX for permute shift, width is the vector element size.
>>>        idx = idx * width.  */
>>> -  emit_insn (gen_ashlsi3 (tmp, idx, GEN_INT (shift)));
>>> -
>>> -  tmp = convert_modes (DImode, SImode, tmp, 1);
>>> +  emit_insn (gen_ashldi3 (tmp, tmp, GEN_INT (shift)));
>>
>> This looks broken.
> 
> Yup.
> 
>> The gen_reg_rtx (DImode); call result is completely ignored,
>> so it wastes one pseudo, and I'm not convinced that idx nor tmp
>> is guaranteed to be usable as output of shift.
>> So, shouldn't it be instead:
>>    rtx tmp = gen_reg_rtx (DImode);
>>    if (idx_mode != DImode)
>>      idx = convert_modes (DImode, idx_mode, idx, 0);
>>
>> ...
>>    emit_insn (gen_ashldi3 (tmp, idx, GEN_INT (shift));
>> ?
> 
> Yeah, something like that.
> 
> Just
>    idx = convert_modes (DImode, idx_mode, idx, 1);
> ...
>    rtx tmp = gen_reg_rtx (DImode);
>    emit_insn (gen_ashldi3 (tmp, idx, GEN_INT (shift));
> works just as well.
> 
>> Also, dunno what is less and what is more expensive on
>> rs6000, whether convert_modes with unsigned = 0 or 1.
> 
> Unsigned is never more expensive.  Sometimes it is cheaper.  It depends
> more on the situation what is best.  Just look at the generated code?
> 

Change it to "idx = convert_modes (DImode, idx_mode, idx, 1);"

For the case with "-mvsx -Og -mcpu=power9":

vector char f2(vector char v, char k, char val)
{
  v[k] = val;
  return v;
}

gimple:
_1 = (int) k_2(D);
_7 = v;
_8 = .VEC_SET (_7, val_4(D), _1);
v = _8;
_6 = v;

the expanded RTL is:

    6: NOTE_INSN_BASIC_BLOCK 2
    2: r121:V16QI=%2:V16QI
    3: r122:DI=%5:DI
    4: r123:DI=%6:DI
    5: NOTE_INSN_FUNCTION_BEG
    8: r117:DI=sign_extend(r122:DI#0)
    9: r124:V16QI=r121:V16QI
   10: r126:QI=r123:DI#0
   11: r127:DI=zero_extend(r117:DI#0)
   12: r128:DI=r127:DI<<0
   13: r129:V16QI=unspec[r128:DI] 152
   14: r130:V16QI=unspec[r128:DI] 151
   15: r124:V16QI=unspec[r124:V16QI,r124:V16QI,r129:V16QI] 236
   16: r124:V16QI=unspec[r124:V16QI,r126:QI,0] 118
   17: r124:V16QI=unspec[r124:V16QI,r124:V16QI,r130:V16QI] 236
   18: r119:V16QI=r124:V16QI
   19: r121:V16QI=r119:V16QI
   20: r120:V16QI=r121:V16QI
   24: %2:V16QI=r120:V16QI
   25: use %2:V16QI

sign_extend of insn #8 is generated by "_1 = (int) k_2(D);", zero_extend
of insn #11 is the convert added by this patch.  Both will be optimized
out in final ASM.  Since k is index of vector element, so it could only
be unsigned value, which means zero_extend should be used here as well?

PS: If type of k is "unsigned int", no zero_extend in expander,
and if k is "int", a zero_extend is generated in expander.

Attached the updated patch.  Thanks.


Xionghu

[-- Attachment #2: 0001-rs6000-Convert-the-vector-set-variable-idx-to-DImode.patch --]
[-- Type: text/plain, Size: 4232 bytes --]

From 06bff74a35ad7641dffc87d1eab9c5872851ae79 Mon Sep 17 00:00:00 2001
From: Xionghu Luo <luoxhu@linux.ibm.com>
Date: Tue, 16 Mar 2021 21:23:14 -0500
Subject: [PATCH] rs6000: Convert the vector set variable idx to DImode
 [PR98914]

vec_insert defines the element argument type to be signed int by ELFv2
ABI.  When expanding a vector with a variable rtx, convert the rtx type
to DImode to support both intrinsic usage and other callers from
rs6000_expand_vector_init produced by v[k] = val when k is long type.

gcc/ChangeLog:

2021-03-17  Xionghu Luo  <luoxhu@linux.ibm.com>

	PR target/98914
	* config/rs6000/rs6000.c (rs6000_expand_vector_set_var_p9):
	Convert idx to DImode.
	(rs6000_expand_vector_set_var_p8): Likewise.

gcc/testsuite/ChangeLog:

2021-03-17  Xionghu Luo  <luoxhu@linux.ibm.com>

	PR target/98914
	* gcc.target/powerpc/pr98914.c: New test.
---
 gcc/config/rs6000/rs6000.c                 | 39 ++++++++++------------
 gcc/testsuite/gcc.target/powerpc/pr98914.c | 11 ++++++
 2 files changed, 29 insertions(+), 21 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr98914.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index ec068c58aa5..bb7fcdca876 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -7000,21 +7000,22 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
 
   gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
 
-  gcc_assert (GET_MODE (idx) == E_SImode);
-
   machine_mode inner_mode = GET_MODE (val);
 
-  rtx tmp = gen_reg_rtx (GET_MODE (idx));
   int width = GET_MODE_SIZE (inner_mode);
 
   gcc_assert (width >= 1 && width <= 8);
 
   int shift = exact_log2 (width);
+
+  machine_mode idx_mode = GET_MODE (idx);
+  if (idx_mode != DImode)
+    idx = convert_modes (DImode, idx_mode, idx, 1);
+
   /* Generate the IDX for permute shift, width is the vector element size.
      idx = idx * width.  */
-  emit_insn (gen_ashlsi3 (tmp, idx, GEN_INT (shift)));
-
-  tmp = convert_modes (DImode, SImode, tmp, 1);
+  rtx tmp = gen_reg_rtx (DImode);
+  emit_insn (gen_ashldi3 (tmp, idx, GEN_INT (shift)));
 
   /*  lvsr    v1,0,idx.  */
   rtx pcvr = gen_reg_rtx (V16QImode);
@@ -7047,28 +7048,25 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
 
   gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
 
-  gcc_assert (GET_MODE (idx) == E_SImode);
-
   machine_mode inner_mode = GET_MODE (val);
   HOST_WIDE_INT mode_mask = GET_MODE_MASK (inner_mode);
 
-  rtx tmp = gen_reg_rtx (GET_MODE (idx));
   int width = GET_MODE_SIZE (inner_mode);
-
   gcc_assert (width >= 1 && width <= 4);
 
+  machine_mode idx_mode = GET_MODE (idx);
+  if (idx_mode != DImode)
+    idx = convert_modes (DImode, idx_mode, idx, 1);
+
+  /*  idx = idx * width.  */
+  rtx tmp = gen_reg_rtx (DImode);
+  emit_insn (gen_muldi3 (tmp, idx, GEN_INT (width)));
+
+  /*  For LE:  idx = idx + 8.  */
   if (!BYTES_BIG_ENDIAN)
-    {
-      /*  idx = idx * width.  */
-      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
-      /*  idx = idx + 8.  */
-      emit_insn (gen_addsi3 (tmp, tmp, GEN_INT (8)));
-    }
+    emit_insn (gen_adddi3 (tmp, tmp, GEN_INT (8)));
   else
-    {
-      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
-      emit_insn (gen_subsi3 (tmp, GEN_INT (24 - width), tmp));
-    }
+    emit_insn (gen_subdi3 (tmp, GEN_INT (24 - width), tmp));
 
   /*  lxv vs33, mask.
       DImode: 0xffffffffffffffff0000000000000000
@@ -7118,7 +7116,6 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
   emit_insn (gen_rtx_SET (val_v16qi, sub_val));
 
   /*  lvsl    13,0,idx.  */
-  tmp = convert_modes (DImode, SImode, tmp, 1);
   rtx pcv = gen_reg_rtx (V16QImode);
   emit_insn (gen_altivec_lvsl_reg (pcv, tmp));
 
diff --git a/gcc/testsuite/gcc.target/powerpc/pr98914.c b/gcc/testsuite/gcc.target/powerpc/pr98914.c
new file mode 100644
index 00000000000..e4d78e3e6b3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr98914.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-options "-Og -mvsx" } */
+
+vector int
+foo (vector int v)
+{
+  for (long k = 0; k < 1; ++k)
+    v[k] = 0;
+  return v;
+}
-- 
2.25.1

Re: Ping^2: [PATCH v2] rs6000: Convert the vector element register to SImode [PR98914]

[Jakub Jelinek]

On Wed, Mar 17, 2021 at 11:35:18AM +0800, Xionghu Luo wrote:
> +  machine_mode idx_mode = GET_MODE (idx);
> +  if (idx_mode != DImode)
> +    idx = convert_modes (DImode, idx_mode, idx, 1);

Segher mentioned you can remove the if (idx_mode != DImode) too,
convert_modes has an early if (mode == oldmode) return x;

> +
> +  /*  idx = idx * width.  */
> +  rtx tmp = gen_reg_rtx (DImode);
> +  emit_insn (gen_muldi3 (tmp, idx, GEN_INT (width)));

And I have to wonder, when rs6000_expand_vector_set_var_p9
uses here a left shift, can't rs6000_expand_vector_set_var_p8
do the same or is there some reason I'm missing?
gen_muldi3 will just emit a multiplication, are you hoping that
combiner or some other pass will optimize it away later (for width 1)
or change the multiplication into left shift otherwise?

	Jakub

Re: Ping^2: [PATCH v2] rs6000: Convert the vector element register to SImode [PR98914]

[Xionghu Luo]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]



On 2021/3/17 15:53, Jakub Jelinek wrote:
> On Wed, Mar 17, 2021 at 11:35:18AM +0800, Xionghu Luo wrote:
>> +  machine_mode idx_mode = GET_MODE (idx);
>> +  if (idx_mode != DImode)
>> +    idx = convert_modes (DImode, idx_mode, idx, 1);
> 
> Segher mentioned you can remove the if (idx_mode != DImode) too,
> convert_modes has an early if (mode == oldmode) return x;

OK.

> 
>> +
>> +  /*  idx = idx * width.  */
>> +  rtx tmp = gen_reg_rtx (DImode);
>> +  emit_insn (gen_muldi3 (tmp, idx, GEN_INT (width)));
> 
> And I have to wonder, when rs6000_expand_vector_set_var_p9
> uses here a left shift, can't rs6000_expand_vector_set_var_p8
> do the same or is there some reason I'm missing?
> gen_muldi3 will just emit a multiplication, are you hoping that
> combiner or some other pass will optimize it away later (for width 1)
> or change the multiplication into left shift otherwise?
> 
Yes, should also use shift here to avoid multiplication, thanks for the catch.
Attached updated patch.


Xionghu

[-- Attachment #2: 0001-rs6000-Convert-the-vector-set-variable-idx-to-DImode.patch --]
[-- Type: text/plain, Size: 4212 bytes --]

From 693b40d5f521c9a28e9923bea6d65bf71afa8c01 Mon Sep 17 00:00:00 2001
From: Xionghu Luo <luoxhu@linux.ibm.com>
Date: Tue, 16 Mar 2021 21:23:14 -0500
Subject: [PATCH] rs6000: Convert the vector set variable idx to DImode
 [PR98914]

vec_insert defines the element argument type to be signed int by ELFv2
ABI.  When expanding a vector with a variable rtx, convert the rtx type
to DImode to support both intrinsic usage and other callers from
rs6000_expand_vector_init produced by v[k] = val when k is long type.

gcc/ChangeLog:

2021-03-18  Xionghu Luo  <luoxhu@linux.ibm.com>

	PR target/98914
	* config/rs6000/rs6000.c (rs6000_expand_vector_set_var_p9):
	Convert idx to DImode.
	(rs6000_expand_vector_set_var_p8): Likewise.

gcc/testsuite/ChangeLog:

2021-03-18  Xionghu Luo  <luoxhu@linux.ibm.com>

	PR target/98914
	* gcc.target/powerpc/pr98914.c: New test.
---
 gcc/config/rs6000/rs6000.c                 | 39 ++++++++++------------
 gcc/testsuite/gcc.target/powerpc/pr98914.c | 11 ++++++
 2 files changed, 29 insertions(+), 21 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr98914.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index ec068c58aa5..d5b8a0e00d1 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -7000,21 +7000,21 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
 
   gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
 
-  gcc_assert (GET_MODE (idx) == E_SImode);
-
   machine_mode inner_mode = GET_MODE (val);
 
-  rtx tmp = gen_reg_rtx (GET_MODE (idx));
   int width = GET_MODE_SIZE (inner_mode);
 
   gcc_assert (width >= 1 && width <= 8);
 
   int shift = exact_log2 (width);
+
+  machine_mode idx_mode = GET_MODE (idx);
+  idx = convert_modes (DImode, idx_mode, idx, 1);
+
   /* Generate the IDX for permute shift, width is the vector element size.
      idx = idx * width.  */
-  emit_insn (gen_ashlsi3 (tmp, idx, GEN_INT (shift)));
-
-  tmp = convert_modes (DImode, SImode, tmp, 1);
+  rtx tmp = gen_reg_rtx (DImode);
+  emit_insn (gen_ashldi3 (tmp, idx, GEN_INT (shift)));
 
   /*  lvsr    v1,0,idx.  */
   rtx pcvr = gen_reg_rtx (V16QImode);
@@ -7047,28 +7047,26 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
 
   gcc_assert (VECTOR_MEM_VSX_P (mode) && !CONST_INT_P (idx));
 
-  gcc_assert (GET_MODE (idx) == E_SImode);
-
   machine_mode inner_mode = GET_MODE (val);
   HOST_WIDE_INT mode_mask = GET_MODE_MASK (inner_mode);
 
-  rtx tmp = gen_reg_rtx (GET_MODE (idx));
   int width = GET_MODE_SIZE (inner_mode);
-
   gcc_assert (width >= 1 && width <= 4);
 
+  int shift = exact_log2 (width);
+
+  machine_mode idx_mode = GET_MODE (idx);
+  idx = convert_modes (DImode, idx_mode, idx, 1);
+
+  /*  idx = idx * width.  */
+  rtx tmp = gen_reg_rtx (DImode);
+  emit_insn (gen_ashldi3 (tmp, idx, GEN_INT (shift)));
+
+  /*  For LE:  idx = idx + 8.  */
   if (!BYTES_BIG_ENDIAN)
-    {
-      /*  idx = idx * width.  */
-      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
-      /*  idx = idx + 8.  */
-      emit_insn (gen_addsi3 (tmp, tmp, GEN_INT (8)));
-    }
+    emit_insn (gen_adddi3 (tmp, tmp, GEN_INT (8)));
   else
-    {
-      emit_insn (gen_mulsi3 (tmp, idx, GEN_INT (width)));
-      emit_insn (gen_subsi3 (tmp, GEN_INT (24 - width), tmp));
-    }
+    emit_insn (gen_subdi3 (tmp, GEN_INT (24 - width), tmp));
 
   /*  lxv vs33, mask.
       DImode: 0xffffffffffffffff0000000000000000
@@ -7118,7 +7116,6 @@ rs6000_expand_vector_set_var_p8 (rtx target, rtx val, rtx idx)
   emit_insn (gen_rtx_SET (val_v16qi, sub_val));
 
   /*  lvsl    13,0,idx.  */
-  tmp = convert_modes (DImode, SImode, tmp, 1);
   rtx pcv = gen_reg_rtx (V16QImode);
   emit_insn (gen_altivec_lvsl_reg (pcv, tmp));
 
diff --git a/gcc/testsuite/gcc.target/powerpc/pr98914.c b/gcc/testsuite/gcc.target/powerpc/pr98914.c
new file mode 100644
index 00000000000..e4d78e3e6b3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr98914.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-options "-Og -mvsx" } */
+
+vector int
+foo (vector int v)
+{
+  for (long k = 0; k < 1; ++k)
+    v[k] = 0;
+  return v;
+}
-- 
2.25.1

Re: Ping^2: [PATCH v2] rs6000: Convert the vector element register to SImode [PR98914]

[Jakub Jelinek]

On Thu, Mar 18, 2021 at 09:27:17AM +0800, Xionghu Luo via Gcc-patches wrote:
> gcc/ChangeLog:
> 
> 2021-03-18  Xionghu Luo  <luoxhu@linux.ibm.com>
> 
> 	PR target/98914
> 	* config/rs6000/rs6000.c (rs6000_expand_vector_set_var_p9):
> 	Convert idx to DImode.
> 	(rs6000_expand_vector_set_var_p8): Likewise.
> 
> gcc/testsuite/ChangeLog:
> 
> 2021-03-18  Xionghu Luo  <luoxhu@linux.ibm.com>
> 
> 	PR target/98914
> 	* gcc.target/powerpc/pr98914.c: New test.

LGTM.  But please give Segher some time to chime in if he disagrees.

	Jakub

Re: Ping^2: [PATCH v2] rs6000: Convert the vector element register to SImode [PR98914]

[Segher Boessenkool]

On Thu, Mar 18, 2021 at 05:13:38PM +0100, Jakub Jelinek wrote:
> On Thu, Mar 18, 2021 at 09:27:17AM +0800, Xionghu Luo via Gcc-patches wrote:
> > gcc/ChangeLog:
> > 
> > 2021-03-18  Xionghu Luo  <luoxhu@linux.ibm.com>
> > 
> > 	PR target/98914
> > 	* config/rs6000/rs6000.c (rs6000_expand_vector_set_var_p9):
> > 	Convert idx to DImode.
> > 	(rs6000_expand_vector_set_var_p8): Likewise.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 2021-03-18  Xionghu Luo  <luoxhu@linux.ibm.com>
> > 
> > 	PR target/98914
> > 	* gcc.target/powerpc/pr98914.c: New test.
> 
> LGTM.  But please give Segher some time to chime in if he disagrees.

It looks fine to me as well.  Okay for trunk.  Thanks!


Segher