* [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] @ 2024-02-26 6:18 jeevitha 2024-02-26 10:49 ` Kewen.Lin 2024-02-26 20:32 ` [PATCH V2] " jeevitha 0 siblings, 2 replies; 14+ messages in thread From: jeevitha @ 2024-02-26 6:18 UTC (permalink / raw) To: GCC Patches, Kewen.Lin, Segher Boessenkool, Michael Meissner Cc: Peter Bergner 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. 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]; 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; +} ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-26 6:18 [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] jeevitha @ 2024-02-26 10:49 ` Kewen.Lin 2024-02-26 15:07 ` Peter Bergner 2024-02-26 20:32 ` [PATCH V2] " jeevitha 1 sibling, 1 reply; 14+ messages in thread From: Kewen.Lin @ 2024-02-26 10:49 UTC (permalink / raw) To: jeevitha; +Cc: Peter Bergner, GCC Patches, Segher Boessenkool, Michael Meissner 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; > +} > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-26 10:49 ` Kewen.Lin @ 2024-02-26 15:07 ` Peter Bergner 2024-02-26 15:12 ` jeevitha 2024-02-27 1:55 ` Kewen.Lin 0 siblings, 2 replies; 14+ messages in thread From: Peter Bergner @ 2024-02-26 15:07 UTC (permalink / raw) To: Kewen.Lin, jeevitha; +Cc: GCC Patches, Segher Boessenkool, Michael Meissner 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-26 15:07 ` Peter Bergner @ 2024-02-26 15:12 ` jeevitha 2024-02-27 1:55 ` Kewen.Lin 1 sibling, 0 replies; 14+ messages in thread From: jeevitha @ 2024-02-26 15:12 UTC (permalink / raw) To: Peter Bergner, Kewen.Lin Cc: GCC Patches, Segher Boessenkool, Michael Meissner 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-26 15:07 ` Peter Bergner 2024-02-26 15:12 ` jeevitha @ 2024-02-27 1:55 ` Kewen.Lin 2024-02-27 2:13 ` Peter Bergner 1 sibling, 1 reply; 14+ messages in thread From: Kewen.Lin @ 2024-02-27 1:55 UTC (permalink / raw) To: Peter Bergner, jeevitha; +Cc: GCC Patches, Segher Boessenkool, Michael Meissner 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-27 1:55 ` Kewen.Lin @ 2024-02-27 2:13 ` Peter Bergner 2024-02-27 2:56 ` Kewen.Lin 0 siblings, 1 reply; 14+ messages in thread From: Peter Bergner @ 2024-02-27 2:13 UTC (permalink / raw) To: Kewen.Lin, jeevitha; +Cc: GCC Patches, Segher Boessenkool, Michael Meissner 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-27 2:13 ` Peter Bergner @ 2024-02-27 2:56 ` Kewen.Lin 2024-02-27 9:07 ` jeevitha 0 siblings, 1 reply; 14+ messages in thread From: Kewen.Lin @ 2024-02-27 2:56 UTC (permalink / raw) To: Peter Bergner, jeevitha; +Cc: GCC Patches, Segher Boessenkool, Michael Meissner 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-27 2:56 ` Kewen.Lin @ 2024-02-27 9:07 ` jeevitha 0 siblings, 0 replies; 14+ messages in thread From: jeevitha @ 2024-02-27 9:07 UTC (permalink / raw) To: Kewen.Lin, Peter Bergner Cc: GCC Patches, Segher Boessenkool, Michael Meissner 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-26 6:18 [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] jeevitha 2024-02-26 10:49 ` Kewen.Lin @ 2024-02-26 20:32 ` jeevitha 2024-02-27 12:40 ` Segher Boessenkool 1 sibling, 1 reply; 14+ messages in thread From: jeevitha @ 2024-02-26 20:32 UTC (permalink / raw) To: GCC Patches, Kewen.Lin, Segher Boessenkool, Michael Meissner Cc: Peter Bergner Hi All, The following patch has been bootstrapped and regtested on powerpc64le-linux. There is no immediate value splatting instruction in Power. Currently, those values need to be stored in a register or memory. To address this issue, I have updated the predicate for the second operand in vsx_splat to splat_input_operand and corrected the assignment of op1 to operands[1]. These changes ensure that operand1 is stored in a register. 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 and corrected the assignment. 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..3e2df247630 100644 --- a/gcc/config/rs6000/vsx.md +++ b/gcc/config/rs6000/vsx.md @@ -4660,14 +4660,14 @@ (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]; if (MEM_P (op1)) operands[1] = rs6000_force_indexed_or_indirect_mem (op1); else if (!REG_P (op1)) - op1 = force_reg (<VSX_D:VEC_base>mode, op1); + operands[1] = force_reg (<VSX_D:VEC_base>mode, op1); }) (define_insn "vsx_splat_<mode>_reg" diff --git a/gcc/testsuite/gcc.target/powerpc/pr113950.c b/gcc/testsuite/gcc.target/powerpc/pr113950.c new file mode 100644 index 00000000000..5c6865a8544 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr113950.c @@ -0,0 +1,25 @@ +/* PR target/113950 */ +/* { dg-do compile } */ +/* { dg-require-effective-target powerpc_vsx_ok } */ +/* { dg-options "-O1 -mdejagnu-cpu=power7" } */ + +/* 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; +} ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-26 20:32 ` [PATCH V2] " jeevitha @ 2024-02-27 12:40 ` Segher Boessenkool 2024-02-27 22:50 ` Peter Bergner 0 siblings, 1 reply; 14+ messages in thread From: Segher Boessenkool @ 2024-02-27 12:40 UTC (permalink / raw) To: jeevitha; +Cc: GCC Patches, Kewen.Lin, Michael Meissner, Peter Bergner Hi! On Tue, Feb 27, 2024 at 02:02:38AM +0530, jeevitha wrote: > There is no immediate value splatting instruction in Power. Currently, those > values need to be stored in a register or memory. To address this issue, I > have updated the predicate for the second operand in vsx_splat to > splat_input_operand and corrected the assignment of op1 to operands[1]. > These changes ensure that operand1 is stored in a register. input_operand allows a lot of things that splat_input_operand does not, not just immediate operands. NAK. (For example, *all* memory is okay for input_operand, always). I'm not saying we do not want to restrict these things, but a commit that doesn't discuss this at all is not okay. Sorry. Segher ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-27 12:40 ` Segher Boessenkool @ 2024-02-27 22:50 ` Peter Bergner 2024-02-28 14:31 ` Segher Boessenkool 0 siblings, 1 reply; 14+ messages in thread From: Peter Bergner @ 2024-02-27 22:50 UTC (permalink / raw) To: Segher Boessenkool, jeevitha; +Cc: GCC Patches, Kewen.Lin, Michael Meissner On 2/27/24 6:40 AM, Segher Boessenkool wrote: > On Tue, Feb 27, 2024 at 02:02:38AM +0530, jeevitha wrote: >> There is no immediate value splatting instruction in Power. Currently, those >> values need to be stored in a register or memory. To address this issue, I >> have updated the predicate for the second operand in vsx_splat to >> splat_input_operand and corrected the assignment of op1 to operands[1]. >> These changes ensure that operand1 is stored in a register. > > input_operand allows a lot of things that splat_input_operand does not, > not just immediate operands. NAK. > > (For example, *all* memory is okay for input_operand, always). > > I'm not saying we do not want to restrict these things, but a commit > that doesn't discuss this at all is not okay. Sorry. So it seems you're not NAKing the use of splat_input_operand, but just that it needs more explanation in the git log entry, correct? Yes, input_operand accepts a lot more things than splat_input_operand does, but the multiple define_insns this define_expand feeds, uses gpc_reg_operand, memory_operand and splat_input_operand for their operands[1] operand (splat_input_operand accepts reg and mem too), so it seems to match better what the patterns will be accepting and I always thought that using predicates that more accurately reflect what the define_insns expect/accept lead to better code gen. Mike, was it just an oversight to not use splat_input_operand for the vsx_splat_<mode> expander or was input_operand a conscious decision? If input_operand was used purposely, then we can just fall back to the s/op1/operands[1]/ change which we already know fixes the bug. Peter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-27 22:50 ` Peter Bergner @ 2024-02-28 14:31 ` Segher Boessenkool 2024-02-28 17:58 ` Peter Bergner 0 siblings, 1 reply; 14+ messages in thread From: Segher Boessenkool @ 2024-02-28 14:31 UTC (permalink / raw) To: Peter Bergner; +Cc: jeevitha, GCC Patches, Kewen.Lin, Michael Meissner On Tue, Feb 27, 2024 at 04:50:02PM -0600, Peter Bergner wrote: > On 2/27/24 6:40 AM, Segher Boessenkool wrote: > > On Tue, Feb 27, 2024 at 02:02:38AM +0530, jeevitha wrote: > > input_operand allows a lot of things that splat_input_operand does not, > > not just immediate operands. NAK. > > > > (For example, *all* memory is okay for input_operand, always). > > > > I'm not saying we do not want to restrict these things, but a commit > > that doesn't discuss this at all is not okay. Sorry. > > So it seems you're not NAKing the use of splat_input_operand, but > just that it needs more explanation in the git log entry, correct? I NAK the patch. _Of course_ there needs to be *something* done, there is a bug after all, it needs to be fixed. But no, there are big questions about if splat_input_operand is correct as well. This needs to be justified in the patch submission. > Yes, input_operand accepts a lot more things than splat_input_operand > does, but the multiple define_insns this define_expand feeds, uses > gpc_reg_operand, memory_operand and splat_input_operand for their > operands[1] operand (splat_input_operand accepts reg and mem too), > so it seems to match better what the patterns will be accepting and > I always thought that using predicates that more accurately reflect > what the define_insns expect/accept lead to better code gen. Still, it needs explanation why we allowed it before, but that was a mistake, or for some reason we do not need it. Sell your patch! :-) > Mike, was it just an oversight to not use splat_input_operand for the > vsx_splat_<mode> expander or was input_operand a conscious decision? > > If input_operand was used purposely, then we can just fall back to > the s/op1/operands[1]/ change which we already know fixes the bug. input_operand allows all inputs for mov insns. It isn't suitable for any other instructions. Segher ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-28 14:31 ` Segher Boessenkool @ 2024-02-28 17:58 ` Peter Bergner 2024-02-28 20:00 ` Segher Boessenkool 0 siblings, 1 reply; 14+ messages in thread From: Peter Bergner @ 2024-02-28 17:58 UTC (permalink / raw) To: Segher Boessenkool; +Cc: jeevitha, GCC Patches, Kewen.Lin, Michael Meissner On 2/28/24 8:31 AM, Segher Boessenkool wrote: > On Tue, Feb 27, 2024 at 04:50:02PM -0600, Peter Bergner wrote: >> So it seems you're not NAKing the use of splat_input_operand, but >> just that it needs more explanation in the git log entry, correct? > > I NAK the patch. _Of course_ there needs to be *something* done, there > is a bug after all, it needs to be fixed. > > But no, there are big questions about if splat_input_operand is correct > as well. This needs to be justified in the patch submission. Ok, then Jeevitha, repost the patch with the s/op1/operands[1]/ only change. Jeevitha has already bootstrapped and regtested that change and it does fix the bug. Clearly, the splat_input_operand change needs more discussion and would be a follow-on patch...if we decide to do it at all. Peter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] 2024-02-28 17:58 ` Peter Bergner @ 2024-02-28 20:00 ` Segher Boessenkool 0 siblings, 0 replies; 14+ messages in thread From: Segher Boessenkool @ 2024-02-28 20:00 UTC (permalink / raw) To: Peter Bergner; +Cc: jeevitha, GCC Patches, Kewen.Lin, Michael Meissner On Wed, Feb 28, 2024 at 11:58:15AM -0600, Peter Bergner wrote: > On 2/28/24 8:31 AM, Segher Boessenkool wrote: > > On Tue, Feb 27, 2024 at 04:50:02PM -0600, Peter Bergner wrote: > >> So it seems you're not NAKing the use of splat_input_operand, but > >> just that it needs more explanation in the git log entry, correct? > > > > I NAK the patch. _Of course_ there needs to be *something* done, there > > is a bug after all, it needs to be fixed. > > > > But no, there are big questions about if splat_input_operand is correct > > as well. This needs to be justified in the patch submission. > > Ok, then Jeevitha, repost the patch with the s/op1/operands[1]/ only change. > Jeevitha has already bootstrapped and regtested that change and it does > fix the bug. > > Clearly, the splat_input_operand change needs more discussion and would > be a follow-on patch...if we decide to do it at all. It is clear that input_operand is wrong. It isn't clear that splat_input_operand is correct though :-( Segher ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-02-28 20:01 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-26 6:18 [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950] jeevitha 2024-02-26 10:49 ` Kewen.Lin 2024-02-26 15:07 ` Peter Bergner 2024-02-26 15:12 ` jeevitha 2024-02-27 1:55 ` Kewen.Lin 2024-02-27 2:13 ` Peter Bergner 2024-02-27 2:56 ` Kewen.Lin 2024-02-27 9:07 ` jeevitha 2024-02-26 20:32 ` [PATCH V2] " jeevitha 2024-02-27 12:40 ` Segher Boessenkool 2024-02-27 22:50 ` Peter Bergner 2024-02-28 14:31 ` Segher Boessenkool 2024-02-28 17:58 ` Peter Bergner 2024-02-28 20:00 ` Segher Boessenkool
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).