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

[jeevitha <jeevitha@linux.vnet.ibm.com>]

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.