Re: [PATCH] rs6000: Fix condition of define_expand vec_shr_ [PR100645]
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_ >> 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_" >>[(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)" > >> --- /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
Re: [PATCH] rs6000: Fix condition of define_expand vec_shr_ [PR100645]
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_ > 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_" >[(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)" > --- /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
[PATCH] rs6000: Fix condition of define_expand vec_shr_ [PR100645]
Hi, PR100645 exposes one latent bug in define_expand vec_shr_ 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_): 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 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_" [(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)" { 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 000..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}); +} + -- 2.27.0