Re: [PATCH] rs6000: Fix vector_set_var_p9 by considering BE [PR108807]

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Segher,

Thanks for the review!

on 2023/4/3 19:44, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Feb 17, 2023 at 05:55:04PM +0800, Kewen.Lin wrote:
>> As PR108807 exposes, the current handling in function
>> rs6000_expand_vector_set_var_p9 doesn't take care of big
>> endianness.  Currently the function is to rotate the
>> target vector by moving element to-be-set to element 0,
>> set element 0 with the given val, then rotate back.  To
>> get the permutation control vector for the rotation, it
>> makes use of lvsr and lvsl, but the element ordering is
>> different for BE and LE (like element 0 is the most
>> significant one on BE while the least significant one on
>> LE), this patch is to add consideration for BE and make
>> sure permutation control vectors for rotations are expected.
> 
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -7235,22 +7235,26 @@ rs6000_expand_vector_set_var_p9 (rtx target, rtx val, rtx idx)
>>
>>    machine_mode shift_mode;
>>    rtx (*gen_ashl)(rtx, rtx, rtx);
>> -  rtx (*gen_lvsl)(rtx, rtx);
>> -  rtx (*gen_lvsr)(rtx, rtx);
>> +  rtx (*gen_pcvr1)(rtx, rtx);
>> +  rtx (*gen_pcvr2)(rtx, rtx);
> 
> Space before "(" btw, you can fix that at the same time? :-)
> 

Good catch, fixed.

> What does "pcvr" mean?  You could put that in a short comment?
> 
>> +  /* Generate one permutation control vector used for rotating the element
> 
> Ah.  Yeah just "/* Permutation control vector */" for the above one
> prevents all mysteries :-)

One comment line added for gen_* function pointers.

> 
> Patch looks good.  Thanks!
> 

Pushed as r13-6994-gd634e6088f139e, thanks!

BR,
Kewen