[V3] rs6000: Don't ICE when compiling the __builtin_vsx_splat_2di built-in [PR113950]

Hi All,

The following patch has been bootstrapped and regtested on powerpc64le-linux.
										
When we expand the __builtin_vsx_splat_2di function, we were allowing immediate
value for second operand which causes an unrecognizable insn ICE. Even though
the immediate value was forced into a register, it wasn't correctly assigned
to the second operand. So corrected the assignment of op1 to operands[1].

2024-02-29  Jeevitha Palanisamy  <jeevitha@linux.ibm.com>

gcc/
	PR target/113950
	* config/rs6000/vsx.md (vsx_splat_<mode>): Corrected assignment to
	operand1.

gcc/testsuite/
	PR target/113950
	* gcc.target/powerpc/pr113950.c: New testcase.

Hi,

on 2024/3/4 02:55, jeevitha wrote:
> Hi All,
> 
> The following patch has been bootstrapped and regtested on powerpc64le-linux.
> 										
> When we expand the __builtin_vsx_splat_2di function, we were allowing immediate
> value for second operand which causes an unrecognizable insn ICE. Even though
> the immediate value was forced into a register, it wasn't correctly assigned
> to the second operand. So corrected the assignment of op1 to operands[1].
> 
> 2024-02-29  Jeevitha Palanisamy  <jeevitha@linux.ibm.com>
> 
> gcc/
> 	PR target/113950
> 	* config/rs6000/vsx.md (vsx_splat_<mode>): Corrected assignment to
> 	operand1.

Nit: s/Corrected/Correct/, maybe add "and simplify else if with else.".

> 
> 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..f135fa079bd 100644
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -4666,8 +4666,8 @@
>    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);
> +  else
> +    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..64566a580d9
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr113950.c
> @@ -0,0 +1,24 @@
> +/* PR target/113950 */
> +/* { dg-require-effective-target powerpc_vsx_ok } */
> +/* { dg-options "-O1 -mvsx" } */

Nit: s/-O1/-O2/, -O2 is preferred when the failure can be reproduced
with -O2 (not just -O1), since optimization may change the level in
which it's placed, -O2 is more general.

As the discussions in the thread of the previous versions, I think
Segher agreed this approach, so OK for trunk with the above nits
tweaked, thanks!

BR,
Kewen

> +
> +/* 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 3/6/24 3:27 AM, Kewen.Lin wrote:
> on 2024/3/4 02:55, jeevitha wrote:
>> The following patch has been bootstrapped and regtested on powerpc64le-linux.
>> 										
>> When we expand the __builtin_vsx_splat_2di function, we were allowing immediate
>> value for second operand which causes an unrecognizable insn ICE. Even though
>> the immediate value was forced into a register, it wasn't correctly assigned
>> to the second operand. So corrected the assignment of op1 to operands[1].
[snip]
> As the discussions in the thread of the previous versions, I think
> Segher agreed this approach, so OK for trunk with the above nits
> tweaked, thanks!

The bogus vsx_splat_<mode> code goes all the way back to GCC 8, so we
should backport this fix.  Segher and Ke Wen, can we get an approval
to backport this to all the open release branches (GCC 13, 12, 11)?
Thanks.

Jeevitha, once we get approval, please perform the backports.

Peter

Hi,

on 2024/3/16 04:34, Peter Bergner wrote:
> On 3/6/24 3:27 AM, Kewen.Lin wrote:
>> on 2024/3/4 02:55, jeevitha wrote:
>>> The following patch has been bootstrapped and regtested on powerpc64le-linux.
>>> 										
>>> When we expand the __builtin_vsx_splat_2di function, we were allowing immediate
>>> value for second operand which causes an unrecognizable insn ICE. Even though
>>> the immediate value was forced into a register, it wasn't correctly assigned
>>> to the second operand. So corrected the assignment of op1 to operands[1].
> [snip]
>> As the discussions in the thread of the previous versions, I think
>> Segher agreed this approach, so OK for trunk with the above nits
>> tweaked, thanks!
> 
> The bogus vsx_splat_<mode> code goes all the way back to GCC 8, so we
> should backport this fix.  Segher and Ke Wen, can we get an approval
> to backport this to all the open release branches (GCC 13, 12, 11)?
> Thanks.

Sure, okay for backporting this to all active branches, thanks!

> 
> Jeevitha, once we get approval, please perform the backports.
> 
> Peter
> 
> 

BR,
Kewen

Hi,

On 18/03/24 7:00 am, Kewen.Lin wrote:

>> The bogus vsx_splat_<mode> code goes all the way back to GCC 8, so we
>> should backport this fix.  Segher and Ke Wen, can we get an approval
>> to backport this to all the open release branches (GCC 13, 12, 11)?
>> Thanks.
> 
> Sure, okay for backporting this to all active branches, thanks!
> 

I need clarification regarding the backporting of PR113950 to GCC 12.

We encountered an issue while resolving merge conflicts in GCC 12. The 
problem lies in extra deletions in the diff due to cherry-picking. Now,
we're unsure about the best approach for handling the backport.

To provide context, I have included the relevant diff snippet below,

diff --cc gcc/config/rs6000/vsx.md
index c45794fb9ed,f135fa079bd..00000000000
--- a/gcc/config/rs6000/vsx.md
+++ b/gcc/config/rs6000/vsx.md
@@@ -4562,8 -4666,8 +4562,8 @@@
    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:VS_scalar>mode, op1);
+   else
 -    operands[1] = force_reg (<VSX_D:VEC_base>mode, op1);
++    operands[1] = force_reg (<VSX_D:VS_scalar>mode, op1);
  })

I'm seeking your advice on how to proceed with the backport. Do you
think the above change is acceptable, or should we also backport Segher's
commit e0e3ce634818b83965b87512938490df4d57f81d, which caused the conflict?.
There was no regression with both of these changes.

Jeevitha.

Hi,

on 2024/4/17 17:05, jeevitha wrote:
> Hi,
> 
> On 18/03/24 7:00 am, Kewen.Lin wrote:
> 
>>> The bogus vsx_splat_<mode> code goes all the way back to GCC 8, so we
>>> should backport this fix.  Segher and Ke Wen, can we get an approval
>>> to backport this to all the open release branches (GCC 13, 12, 11)?
>>> Thanks.
>>
>> Sure, okay for backporting this to all active branches, thanks!
>>
> 
> I need clarification regarding the backporting of PR113950 to GCC 12.
> 
> We encountered an issue while resolving merge conflicts in GCC 12. The 
> problem lies in extra deletions in the diff due to cherry-picking. Now,
> we're unsure about the best approach for handling the backport.
> 
> To provide context, I have included the relevant diff snippet below,
> 
> diff --cc gcc/config/rs6000/vsx.md
> index c45794fb9ed,f135fa079bd..00000000000
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@@ -4562,8 -4666,8 +4562,8 @@@
>     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:VS_scalar>mode, op1);
> +   else
>  -    operands[1] = force_reg (<VSX_D:VEC_base>mode, op1);
> ++    operands[1] = force_reg (<VSX_D:VS_scalar>mode, op1);
>   })
> 
> I'm seeking your advice on how to proceed with the backport. Do you
> think the above change is acceptable, or should we also backport Segher's
> commit e0e3ce634818b83965b87512938490df4d57f81d, which caused the conflict?.

I prefer the former, which is the least modification, for release branches
let's introduce as few changes as possible, and the amendment on the conflict
is minor and straightforward.

BR,
Kewen

> There was no regression with both of these changes.
> 
> Jeevitha.
>