Re: [PATCH] rs6000: Convert the vector element register to SImode [PR98914]
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(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
Re: [PATCH] rs6000: Convert the vector element register to SImode [PR98914]
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(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
>
> * 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
>
> * 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 000..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
Ping: [PATCH] rs6000: Convert the vector element register to SImode [PR98914]
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(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
* 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
* 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 000..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
[PATCH] rs6000: Convert the vector element register to SImode [PR98914]
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(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
* 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
* 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 000..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
