[PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[jeevitha]

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;
+}

Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[Kewen.Lin]

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;
> +}
> 
> 

Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[Peter Bergner]

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

Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[jeevitha]

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.

[PATCH V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[jeevitha]

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;
+}

Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[Kewen.Lin]

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

Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[Peter Bergner]

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

Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[Kewen.Lin]

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

Re: [PATCH] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[jeevitha]

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

Re: [PATCH V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[Segher Boessenkool]

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

Re: [PATCH V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[Peter Bergner]

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

Re: [PATCH V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[Segher Boessenkool]

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

Re: [PATCH V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[Peter Bergner]

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

Re: [PATCH V2] rs6000: Don't allow immediate value in the vsx_splat pattern [PR113950]

[Segher Boessenkool]

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