diff mbox series

rs6000: Fix condition of define_expand vec_shr_<mode> [PR100645]

Message ID 354edbb1-e0b0-07b3-ceb7-456442781b0b@linux.ibm.com
State Accepted, archived
Headers
Series rs6000: Fix condition of define_expand vec_shr_<mode> [PR100645] |

Checks

Context Check Description
snail/gcc-patches-check success Github commit url

Commit Message

Kewen.Lin Sept. 22, 2022, 1:41 a.m. UTC 
  Hi,
 
PR100645 exposes one latent bug in define_expand vec_shr_<mode>
that the current condition TARGET_ALTIVEC is too loose.  The
mode iterator VEC_L contains a few modes, they are not always
supported as vector mode, VECTOR_UNIT_ALTIVEC_OR_VSX_P should
be used like some other VEC_L usages.

Bootstrapped and regtested on powerpc64-linux-gnu P7 and
powerpc64le-linux-gnu P9 and P10.

I'm going to push it a week later if no objections.

BR,
Kewen
-----
	PR target/100645

gcc/ChangeLog:

	* config/rs6000/vector.md (vec_shr_<mode>): Replace condition
	TARGET_ALTIVEC with VECTOR_UNIT_ALTIVEC_OR_VSX_P.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pr100645.c: New test.
---
 gcc/config/rs6000/vector.md                 |  2 +-
 gcc/testsuite/gcc.target/powerpc/pr100645.c | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr100645.c

--
2.27.0

Comments

Segher Boessenkool Sept. 22, 2022, 9:39 p.m. UTC | #1 
Hi!

Heh, I first thought I had mistyped thgew PR #, but it is this one after
all :-)

On Thu, Sep 22, 2022 at 09:41:34AM +0800, Kewen.Lin wrote:
> PR100645 exposes one latent bug in define_expand vec_shr_<mode>
> that the current condition TARGET_ALTIVEC is too loose.  The
> mode iterator VEC_L contains a few modes, they are not always
> supported as vector mode, VECTOR_UNIT_ALTIVEC_OR_VSX_P should
> be used like some other VEC_L usages.

> --- a/gcc/config/rs6000/vector.md
> +++ b/gcc/config/rs6000/vector.md
> @@ -1475,7 +1475,7 @@ (define_expand "vec_shr_<mode>"
>    [(match_operand:VEC_L 0 "vlogical_operand")
>     (match_operand:VEC_L 1 "vlogical_operand")
>     (match_operand:QI 2 "reg_or_short_operand")]
> -  "TARGET_ALTIVEC"
> +  "VECTOR_UNIT_ALTIVEC_OR_VSX_P (<MODE>mode)"

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr100645.c
> @@ -0,0 +1,13 @@
> +/* { dg-require-effective-target powerpc_altivec_ok } */
> +/* { dg-options "-mdejagnu-cpu=power6 -maltivec" } */

This is a strange choice: we normally do not enable VMX on p6.  Just use
p7 instead?  There is no need for altivec_ok in any case, the -mcpu=
guarantees it is satisfied.

> +/* It's to verify no ICE here.  */

"This used to ICE."?

Please commit this now, looks good.  Thanks!


Segher
Kewen.Lin Sept. 26, 2022, 5:41 a.m. UTC | #2 
Hi Segher,

Thanks for the comments!

on 2022/9/23 05:39, Segher Boessenkool wrote:
> Hi!
> 
> Heh, I first thought I had mistyped thgew PR #, but it is this one after
> all :-)
> 
> On Thu, Sep 22, 2022 at 09:41:34AM +0800, Kewen.Lin wrote:
>> PR100645 exposes one latent bug in define_expand vec_shr_<mode>
>> that the current condition TARGET_ALTIVEC is too loose.  The
>> mode iterator VEC_L contains a few modes, they are not always
>> supported as vector mode, VECTOR_UNIT_ALTIVEC_OR_VSX_P should
>> be used like some other VEC_L usages.
> 
>> --- a/gcc/config/rs6000/vector.md
>> +++ b/gcc/config/rs6000/vector.md
>> @@ -1475,7 +1475,7 @@ (define_expand "vec_shr_<mode>"
>>    [(match_operand:VEC_L 0 "vlogical_operand")
>>     (match_operand:VEC_L 1 "vlogical_operand")
>>     (match_operand:QI 2 "reg_or_short_operand")]
>> -  "TARGET_ALTIVEC"
>> +  "VECTOR_UNIT_ALTIVEC_OR_VSX_P (<MODE>mode)"
> 
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr100645.c
>> @@ -0,0 +1,13 @@
>> +/* { dg-require-effective-target powerpc_altivec_ok } */
>> +/* { dg-options "-mdejagnu-cpu=power6 -maltivec" } */
> 
> This is a strange choice: we normally do not enable VMX on p6.  Just use
> p7 instead?  There is no need for altivec_ok in any case, the -mcpu=
> guarantees it is satisfied.

Unfortunately a single power7 doesn't work for this case, since it (VSX) makes
rs6000_vector_mem[TImode] not VECTOR_NONE any more, we need one extra -mno-vsx
to reproduce this.

As you mentioned above, power6 doesn't enable altivec by default, I noticed
altivec_ok excludes some envs like aix 5.3 etc., and also ensures it's fine
to have an explicit maltivec there, so I added it for robustness.

> 
>> +/* It's to verify no ICE here.  */
> 
> "This used to ICE."?

Updated.

> 
> Please commit this now, looks good.  Thanks!
> 

Committed in r13-2844.  Thanks!

BR,
Kewen
diff mbox series

Patch

diff --git a/gcc/config/rs6000/vector.md b/gcc/config/rs6000/vector.md
index a0d33d2f604..0171705803c 100644
--- a/gcc/config/rs6000/vector.md
+++ b/gcc/config/rs6000/vector.md
@@ -1475,7 +1475,7 @@  (define_expand "vec_shr_<mode>"
   [(match_operand:VEC_L 0 "vlogical_operand")
    (match_operand:VEC_L 1 "vlogical_operand")
    (match_operand:QI 2 "reg_or_short_operand")]
-  "TARGET_ALTIVEC"
+  "VECTOR_UNIT_ALTIVEC_OR_VSX_P (<MODE>mode)"
 {
   rtx bitshift = operands[2];
   rtx shift;
diff --git a/gcc/testsuite/gcc.target/powerpc/pr100645.c b/gcc/testsuite/gcc.target/powerpc/pr100645.c
new file mode 100644
index 00000000000..e221287c0f1
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr100645.c
@@ -0,0 +1,13 @@ 
+/* { dg-require-effective-target powerpc_altivec_ok } */
+/* { dg-options "-mdejagnu-cpu=power6 -maltivec" } */
+
+/* It's to verify no ICE here.  */
+
+typedef long long v2di __attribute__ ((vector_size (16)));
+
+v2di
+foo_v2di_l (v2di x)
+{
+  return __builtin_shuffle ((v2di){0, 0}, x, (v2di){3, 0});
+}
+