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
