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.
Carl
