Re: [PATCH] rs6000: Fix __builtin_vec_xst_trunc definition

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

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