on 2023/6/2 04:01, Carl Love wrote:
> On Wed, 2023-05-31 at 12:59 -0500, Peter Bergner wrote:
>> On 5/22/23 4:04 AM, Kewen.Lin wrote:
>>> on 2023/5/11 02:06, Carl Love via Gcc-patches wrote:
>>>> @@ -3161,12 +3161,15 @@
>>>> void __builtin_altivec_tr_stxvrbx (vsq, signed long, signed
>>>> char *);
>>>> TR_STXVRBX vsx_stxvrbx {stvec}
>>>>
>>>> - void __builtin_altivec_tr_stxvrhx (vsq, signed long, signed
>>>> int *);
>>>> + void __builtin_altivec_tr_stxvrhx (vsq, signed long, signed
>>>> short *);
>>>> TR_STXVRHX vsx_stxvrhx {stvec}
>>>>
>>>> - void __builtin_altivec_tr_stxvrwx (vsq, signed long, signed
>>>> short *);
>>>> + void __builtin_altivec_tr_stxvrwx (vsq, signed long, signed
>>>> int *);
>>>> TR_STXVRWX vsx_stxvrwx {stvec}
>>>
>>> Good catching!
>>
>> This hunk should be its own patch and commit, as it is independent of
>> the other change. Especially since other built-ins also don't have
>> {,un}simgned long * as arguments, not just
>> __builtin_altivec_tr_stxvr*x.
>
> Yes, I was thinking the patch needs to be split into a bug fix and a
> patch for the long * arguments.
>
> I redid the patch to create the bug fix only. The patch includes a
> testcase that tests the __builtin_altivec_tr_stxvr* builtins. I will
> post the new patch.
>
> The updated patch is now called: " rs6000: Fix arguments for
> __builtin_altivec_tr_stxvrwx, __builtin_altivec_tr_stxvrhx"
>
>>
>>
>>
>>>> + void __builtin_altivec_tr_stxvrlx (vsq, signed long, signed
>>>> long *);
>>>> + TR_STXVRLX vsx_stxvrdx {stvec}
>>>> +
>>>
>>> This is mapped to the one used for type long long, it's a hard
>>> mapping,
>>> IMHO it's wrong and not consistent with what the users expect,
>>> since on Power
>>> the size of type long int is 4 bytes at -m32 while 8 bytes at -m64,
>>> this
>>> implementation binding to 8 bytes can cause trouble in 32-bit. I
>>> wonder if
>>> it's a good idea to add one overloaded version for type long int,
>>> for now
>>> openxl also emits error message for long int type pointer (see its
>>> doc [1]),
>>> users can use casting to make it to the acceptable pointer types
>>> (long long
>>> or int as its size).
>>
>> I'm the person who noticed that we don't accept signed/unsigned long
>> * as
>> an argument type and asked Carl to investigate. I find it hard to
>> believe
>> we accept all integer pointer types, except long *. I agree that it
>> shouldn't
>> always map to long long *, since as you say, that's wrong for -m32.
>> My hope was that we could somehow automagically handle the long *
>> types
>> in the built-in machinery, mapping them to either the int * built-in
>> or
>> the long long * built-in depending on -m32 or -m64. Again, this
>> limitation
>> is no limited to __builtin_altivec_tr_stx* built-ins, but others as
>> well,
>> so I was kind of hoping for a general solution that would fix them
>> all.
>> I'm not sure of that's possible though.
>
> Per Peter's request, I added the overloaded version of the
> __builtin_vec_xst_trunc builtin with the long * argument which Kewen
> pushed back on. So, that approach is not acceptable. Not sure about
> how to get the builtin infrastructure to automatically map long * to
> int * or long long *? If someone has some idea on how to do that, I
> will gladly pursue it. I will study the builtin support some more to
> see if I can come up with any ideas as well.
>
Yeah, Peter gave a petty nice idea about a general solution, I think
it's doable. One place is in funciton:
rs6000_builtin_type_compatible (tree parmtype, tree argtype).
For POINTER_TYPE_P (parmtype) && POINTER_TYPE_P (argtype), now we just
check the TREE_TYPE of pointer type, you can do some check on TREE_TYPE
of parmtype with long/unsigned long, and do the corresponding conversion
to unsigned/signed {int, long long} further, then the given instance
can match it, it won't raise errors any more.
But note that not all built-in overloading go into this, since we have
some specific handlers (see resolve_vec_* like mul, cmpne...), you may
need some checks further like this.
Another place is in function altivec_resolve_overloaded_builtin, when
iterating the given fnargs to prepare args and types, we can do the
check and conversion. It's before that we start to route to different
handlers if they need to, it may be better at the first glance.
I noticed that some of built-ins already support {unsigned,} long *
overloaded, once this general solution works, I think we can revisit
and get rid of those ones to speed up the instance searching.
Besides, if we want to support it, we probably want to sync this with
openxl team to keep both compatible. :)
BR,
Kewen
