| Message ID | 7faaa8f2-d706-4f6a-811f-103b74f2de8a@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] | expand |
Hi, on 2024/2/26 14:18, jeevitha wrote: > Hi All, > > The following patch has been bootstrapped and regtested on powerpc64le-linux. > > There is no immediate value splatting instruction in powerpc. Currently that > needs to be stored in a register or memory. For addressing this I have updated > the predicate for the second operand in vsx_splat to splat_input_operand, > which will handle the operands appropriately. The test case fails with error message with GCC 11, but fails with ICE from GCC 12, it's kind of regression, so I think we can make such fix in this stage. Out of curiosity, did you check why it triggers error messages on GCC 11? I guess the difference from GCC 12 is Bill introduced new built-in framework in GCC12 which adds the support for the bif, but I'm curious what prevent this being supported before that. > > 2024-02-26 Jeevitha Palanisamy <jeevitha@linux.ibm.com> > > gcc/ > PR target/113950 > * config/rs6000/vsx.md (vsx_splat_<mode>): Updated the predicates > for second operand. > > gcc/testsuite/ > PR target/113950 > * gcc.target/powerpc/pr113950.c: New testcase. > > diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md > index 6111cc90eb7..e5688ff972a 100644 > --- a/gcc/config/rs6000/vsx.md > +++ b/gcc/config/rs6000/vsx.md > @@ -4660,7 +4660,7 @@ > (define_expand "vsx_splat_<mode>" > [(set (match_operand:VSX_D 0 "vsx_register_operand") > (vec_duplicate:VSX_D > - (match_operand:<VEC_base> 1 "input_operand")))] > + (match_operand:<VEC_base> 1 "splat_input_operand")))] > "VECTOR_MEM_VSX_P (<MODE>mode)" > { > rtx op1 = operands[1]; This hunk actually does force_reg already: ... else if (!REG_P (op1)) op1 = force_reg (<VSX_D:VEC_base>mode, op1); but it's assigning to op1 unexpectedly (an omission IMHO), so just simply fix it with: else if (!REG_P (op1)) - op1 = force_reg (<VSX_D:VEC_base>mode, op1); + operands[1] = force_reg (<VSX_D:VEC_base>mode, op1); instead, can you verify? > diff --git a/gcc/testsuite/gcc.target/powerpc/pr113950.c b/gcc/testsuite/gcc.target/powerpc/pr113950.c > new file mode 100644 > index 00000000000..29ded29f683 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr113950.c > @@ -0,0 +1,24 @@ > +/* PR target/113950 */ > +/* { dg-do compile } */ We need an effective target to ensure vsx support, for now it's powerpc_vsx_ok. ie: /* { dg-require-effective-target powerpc_vsx_ok } */ (most/all of its uses would be replaced with an enhanced powerpc_vsx in next stage 1). BR, Kewen > +/* { dg-options "-O1" } */ > + > +/* Verify we do not ICE on the following. */ > + > +void abort (void); > + > +int main () > +{ > + int i; > + vector signed long long vsll_result, vsll_expected_result; > + signed long long sll_arg1; > + > + sll_arg1 = 300; > + vsll_expected_result = (vector signed long long) {300, 300}; > + vsll_result = __builtin_vsx_splat_2di (sll_arg1); > + > + for (i = 0; i < 2; i++) > + if (vsll_result[i] != vsll_expected_result[i]) > + abort(); > + > + return 0; > +} > >
On 2/26/24 4:49 AM, Kewen.Lin wrote: > on 2024/2/26 14:18, jeevitha wrote: >> Hi All, >> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md >> index 6111cc90eb7..e5688ff972a 100644 >> --- a/gcc/config/rs6000/vsx.md >> +++ b/gcc/config/rs6000/vsx.md >> @@ -4660,7 +4660,7 @@ >> (define_expand "vsx_splat_<mode>" >> [(set (match_operand:VSX_D 0 "vsx_register_operand") >> (vec_duplicate:VSX_D >> - (match_operand:<VEC_base> 1 "input_operand")))] >> + (match_operand:<VEC_base> 1 "splat_input_operand")))] >> "VECTOR_MEM_VSX_P (<MODE>mode)" >> { >> rtx op1 = operands[1]; > > This hunk actually does force_reg already: > > ... > else if (!REG_P (op1)) > op1 = force_reg (<VSX_D:VEC_base>mode, op1); > > but it's assigning to op1 unexpectedly (an omission IMHO), so just > simply fix it with: > > else if (!REG_P (op1)) > - op1 = force_reg (<VSX_D:VEC_base>mode, op1); > + operands[1] = force_reg (<VSX_D:VEC_base>mode, op1); I agree op1 was an oversight and it should be operands[1]. That said, I think using more precise predicates is a good thing, so I think we should use both Jeevitha's predicate change and your operands[1] change. I'll note that Jeevitha originally had the operands[1] change, but I didn't look closely enough at the issue or the pattern and mentioned that these kinds of bugs can be caused by too loose constraints and predicates, which is when she found the updated predicate to use. I believe she already even bootstrapped and regtested the operands[1] only change. Jeevitha??? >> +/* PR target/113950 */ >> +/* { dg-do compile } */ > > We need an effective target to ensure vsx support, for now it's powerpc_vsx_ok. > ie: /* { dg-require-effective-target powerpc_vsx_ok } */ Agreed. >> +/* { dg-options "-O1" } */ I think we should also use a -mcpu=XXX option to ensure VSX is enabled when compiling these VSX built-in functions. I'm fine using any CPU (power7 or later) where the ICE exists with an unpatched compiler. Otherwise, testing will be limited to our server systems that have VSX enabled by default. Peter
On 26/02/24 8:37 pm, Peter Bergner wrote: > On 2/26/24 4:49 AM, Kewen.Lin wrote: >> on 2024/2/26 14:18, jeevitha wrote: >>> Hi All, >>> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md >>> index 6111cc90eb7..e5688ff972a 100644 >>> --- a/gcc/config/rs6000/vsx.md >>> +++ b/gcc/config/rs6000/vsx.md >>> @@ -4660,7 +4660,7 @@ >>> (define_expand "vsx_splat_<mode>" >>> [(set (match_operand:VSX_D 0 "vsx_register_operand") >>> (vec_duplicate:VSX_D >>> - (match_operand:<VEC_base> 1 "input_operand")))] >>> + (match_operand:<VEC_base> 1 "splat_input_operand")))] >>> "VECTOR_MEM_VSX_P (<MODE>mode)" >>> { >>> rtx op1 = operands[1]; >> >> This hunk actually does force_reg already: >> >> ... >> else if (!REG_P (op1)) >> op1 = force_reg (<VSX_D:VEC_base>mode, op1); >> >> but it's assigning to op1 unexpectedly (an omission IMHO), so just >> simply fix it with: >> >> else if (!REG_P (op1)) >> - op1 = force_reg (<VSX_D:VEC_base>mode, op1); >> + operands[1] = force_reg (<VSX_D:VEC_base>mode, op1); > > I agree op1 was an oversight and it should be operands[1]. > That said, I think using more precise predicates is a good thing, > so I think we should use both Jeevitha's predicate change and > your operands[1] change. > > I'll note that Jeevitha originally had the operands[1] change, but I > didn't look closely enough at the issue or the pattern and mentioned > that these kinds of bugs can be caused by too loose constraints and > predicates, which is when she found the updated predicate to use. > I believe she already even bootstrapped and regtested the operands[1] > only change. Jeevitha??? > > Yes, Peter. I have already bootstrapped and regtested the operands[1] change.
on 2024/2/26 23:07, Peter Bergner wrote: > On 2/26/24 4:49 AM, Kewen.Lin wrote: >> on 2024/2/26 14:18, jeevitha wrote: >>> Hi All, >>> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md >>> index 6111cc90eb7..e5688ff972a 100644 >>> --- a/gcc/config/rs6000/vsx.md >>> +++ b/gcc/config/rs6000/vsx.md >>> @@ -4660,7 +4660,7 @@ >>> (define_expand "vsx_splat_<mode>" >>> [(set (match_operand:VSX_D 0 "vsx_register_operand") >>> (vec_duplicate:VSX_D >>> - (match_operand:<VEC_base> 1 "input_operand")))] >>> + (match_operand:<VEC_base> 1 "splat_input_operand")))] >>> "VECTOR_MEM_VSX_P (<MODE>mode)" >>> { >>> rtx op1 = operands[1]; >> >> This hunk actually does force_reg already: >> >> ... >> else if (!REG_P (op1)) >> op1 = force_reg (<VSX_D:VEC_base>mode, op1); >> >> but it's assigning to op1 unexpectedly (an omission IMHO), so just >> simply fix it with: >> >> else if (!REG_P (op1)) >> - op1 = force_reg (<VSX_D:VEC_base>mode, op1); >> + operands[1] = force_reg (<VSX_D:VEC_base>mode, op1); > > I agree op1 was an oversight and it should be operands[1]. > That said, I think using more precise predicates is a good thing, Agreed. > so I think we should use both Jeevitha's predicate change and > your operands[1] change. Since either the original predicate change or operands[1] change can fix this issue, I think it's implied that only either of them is enough, so we can remove "else if (!REG_P (op1))" arm (or even replaced with one else arm to assert REG_P (op1))? > > I'll note that Jeevitha originally had the operands[1] change, but I > didn't look closely enough at the issue or the pattern and mentioned > that these kinds of bugs can be caused by too loose constraints and > predicates, which is when she found the updated predicate to use. > I believe she already even bootstrapped and regtested the operands[1] > only change. Jeevitha??? > Good to know that. :) > > > >>> +/* PR target/113950 */ >>> +/* { dg-do compile } */ >> >> We need an effective target to ensure vsx support, for now it's powerpc_vsx_ok. >> ie: /* { dg-require-effective-target powerpc_vsx_ok } */ > > Agreed. > > >>> +/* { dg-options "-O1" } */ > > I think we should also use a -mcpu=XXX option to ensure VSX is enabled > when compiling these VSX built-in functions. I'm fine using any CPU > (power7 or later) where the ICE exists with an unpatched compiler. > Otherwise, testing will be limited to our server systems that have > VSX enabled by default. Good point, or maybe just an explicit -mvsx like some existing ones, which can avoid to only test some fixed cpu type. BR, Kewen
On 2/26/24 7:55 PM, Kewen.Lin wrote: > on 2024/2/26 23:07, Peter Bergner wrote: >> so I think we should use both Jeevitha's predicate change and >> your operands[1] change. > > Since either the original predicate change or operands[1] change > can fix this issue, I think it's implied that only either of them > is enough, so we can remove "else if (!REG_P (op1))" arm (or even > replaced with one else arm to assert REG_P (op1))? splat_input_operand allows, mem, reg and subreg, so I don't think we can just assert on REG_P (op1), since op1 could be a subreg. I do agree we can remove the "if (!REG_P (op1))" test on the else branch, since force_reg() has an early exit for regs, so a simple: ... else operands[1] = force_reg (<VSX_D:VEC_base>mode, op1); ..should work. > Good point, or maybe just an explicit -mvsx like some existing ones, which > can avoid to only test some fixed cpu type. If a simple "-O1 -vsx" is enough to expose the ICE on an unpacthed compiler and a PASS on a patched compiler, then I'm all for it. Jeevitha, can you try confirming that? Peter
on 2024/2/27 10:13, Peter Bergner wrote: > On 2/26/24 7:55 PM, Kewen.Lin wrote: >> on 2024/2/26 23:07, Peter Bergner wrote: >>> so I think we should use both Jeevitha's predicate change and >>> your operands[1] change. >> >> Since either the original predicate change or operands[1] change >> can fix this issue, I think it's implied that only either of them >> is enough, so we can remove "else if (!REG_P (op1))" arm (or even >> replaced with one else arm to assert REG_P (op1))? > > splat_input_operand allows, mem, reg and subreg, so I don't think > we can just assert on REG_P (op1), since op1 could be a subreg. ah, you are right! I missed the "subreg". > I do agree we can remove the "if (!REG_P (op1))" test on the else > branch, since force_reg() has an early exit for regs, so a simple: > > ... > else > operands[1] = force_reg (<VSX_D:VEC_base>mode, op1); > > ..should work. Yes! > > > > >> Good point, or maybe just an explicit -mvsx like some existing ones, which >> can avoid to only test some fixed cpu type. > > If a simple "-O1 -vsx" is enough to expose the ICE on an unpacthed > compiler and a PASS on a patched compiler, then I'm all for it. > Jeevitha, can you try confirming that? Jeevitha, can you also check why we have the different behavior on GCC 11 when you get time? GCC 12 has new built-in framework, so this ICE gets exposed, but IMHO it would still be good to double check the previous behavior is due to some miss support or some other latent bug. Thanks in advance! BR, Kewen
On 27/02/24 8:26 am, Kewen.Lin wrote: > on 2024/2/27 10:13, Peter Bergner wrote: >> On 2/26/24 7:55 PM, Kewen.Lin wrote: >>> on 2024/2/26 23:07, Peter Bergner wrote: >> >>> Good point, or maybe just an explicit -mvsx like some existing ones, which >>> can avoid to only test some fixed cpu type. >> >> If a simple "-O1 -vsx" is enough to expose the ICE on an unpacthed >> compiler and a PASS on a patched compiler, then I'm all for it. >> Jeevitha, can you try confirming that? Yes, Peter, I've confirmed that using "-O1 -mvsx" is sufficient to expose the issue on the unpatched compiler and ensure successful compilation on the patched one. > > Jeevitha, can you also check why we have the different behavior on GCC 11 when > you get time? GCC 12 has new built-in framework, so this ICE gets exposed, but > IMHO it would still be good to double check the previous behavior is due to > some miss support or some other latent bug. Thanks in advance! Sure Kewen, I will have a look. Jeevitha
diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md index 6111cc90eb7..e5688ff972a 100644 --- a/gcc/config/rs6000/vsx.md +++ b/gcc/config/rs6000/vsx.md @@ -4660,7 +4660,7 @@ (define_expand "vsx_splat_<mode>" [(set (match_operand:VSX_D 0 "vsx_register_operand") (vec_duplicate:VSX_D - (match_operand:<VEC_base> 1 "input_operand")))] + (match_operand:<VEC_base> 1 "splat_input_operand")))] "VECTOR_MEM_VSX_P (<MODE>mode)" { rtx op1 = operands[1]; diff --git a/gcc/testsuite/gcc.target/powerpc/pr113950.c b/gcc/testsuite/gcc.target/powerpc/pr113950.c new file mode 100644 index 00000000000..29ded29f683 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr113950.c @@ -0,0 +1,24 @@ +/* PR target/113950 */ +/* { dg-do compile } */ +/* { dg-options "-O1" } */ + +/* Verify we do not ICE on the following. */ + +void abort (void); + +int main () +{ + int i; + vector signed long long vsll_result, vsll_expected_result; + signed long long sll_arg1; + + sll_arg1 = 300; + vsll_expected_result = (vector signed long long) {300, 300}; + vsll_result = __builtin_vsx_splat_2di (sll_arg1); + + for (i = 0; i < 2; i++) + if (vsll_result[i] != vsll_expected_result[i]) + abort(); + + return 0; +}