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

[Peter Bergner <bergner@linux.ibm.com>]

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