Re: [PATCH 6/13] rs6000, add overloaded vec_sel with int128 arguments

[Carl Love <cel@linux.ibm.com>]

Kewen:

On 5/21/24 20:05, Kewen.Lin wrote:
> Hi Carl,
> 
> on 2024/5/22 08:13, Carl Love wrote:
>> Kewen:

<snip>

>>> Why did you place this in a section for ISA 3.1 (Power10)?  It doesn't really
>>> require this support.  The used instance VSEL_1TI and VSEL_1TI_UNS are placed
>>> in altivec stanza, so it looks that we should put it under the section
>>> "PowerPC AltiVec Built-in Functions on ISA 2.05".  And since it's an extension
>>> of @code{vec_sel} documented in the PVIPR, I prefer to just mention it's "an
>>> extension of the @code{vec_sel} built-in documented in the PVIPR" and omitting
>>> the description to avoid possible slightly different wording.
>>
>> Honestly, at this point in time I don't remember why I put it there.  It has been too long since I created the patch.  That said, the test case requires Power 10 do to the comparison check using built-in vec_all_eq but that is another issue.  
>> The built-in generates the xxsel instruction that is an ISA 2.06 instruction.  So, I would say it should to into the ISA 2.06 section.  I moved it to the ISA 2.06 section.
> 
> But the underlying implementation is:
> 
>   const vsq __builtin_altivec_vsel_1ti (vsq, vsq, vuq);
>     VSEL_1TI vector_select_v1ti {}
> 
>   const vuq __builtin_altivec_vsel_1ti_uns (vuq, vuq, vuq);
>     VSEL_1TI_UNS vector_select_v1ti_uns {}
> 
> , it's under altivec stanza and can result with insn vsel (so not xxsel),
> vsel is ISA 2.03, so I think ISA 2.05 better matches the implementation.

OK, moved to ISA 2.05

> 

<snip>

>>
>> Sounds like there was some issue that you noticed on r14-10011-g6e62ede7aaccc6.  The new version of
>> print_i128 should be functionally equivalent but perhaps is "safer"?
> 
> Thanks for checking!  Looking into this more closely, I realized you didn't apply the previously
> adopted way for printing (the way used in gcc.target/powerpc/builtins-6-p9-runnable.c), sorry for
> the false alarm!  So your supposed print_i128 is fine to me.

OK, no problem.  Will go with the original print_i128 function.

                    Carl