[PATCH] PowerPC support for the ELFv2 ABI (powerpc64le-linux)

[Hal Finkel]

----- Original Message -----
> From: "Ulrich Weigand" <Ulrich.Weigand at de.ibm.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "cfe-commits" <cfe-commits at cs.uiuc.edu>
> Sent: Sunday, July 20, 2014 8:29:05 PM
> Subject: Re: [PATCH] PowerPC support for the ELFv2 ABI (powerpc64le-linux)
> 
> Hal Finkel <hfinkel at anl.gov> wrote on 18.07.2014 22:32:01:
> 
> > > A final note: as with the LLVM series, I have not (yet)
> > > implemented
> > > the
> > > -mabi=elfv1 / -mabi=elfv2 command line arguments.  Instead, we
> > > hard-code
> > > ppc64 to use ELFv1 and ppc64le to use ELFv2.  If desired, it
> > > should
> > > be
> > > straight-forward to implement the option machinery and hook it up
> > > to
> > > the
> > > ELFv2 ABI flag passed to the PPC64_SVR4_TargetCodeGenInfo
> > > constructor.
> >
> > I do think this will be desirable, please submit a follow-up patch
> > for
> that.
> 
> Sure, I'll try to do that within the next couple of days ...
> 
> > Regarding the patch:
> >
> > +private:
> > +  ABIKind Kind;
> > +  ABIKind getABIKind() const { return Kind; }
> >
> > You don't need to introduce a accessor function here if it is
> > trivial and also private.
> 
> I was just following existing precedent in the file, but I'm
> certainly fine with omitting the accessor function as well.
> 
> > +    } else if (const VectorType *VT = Ty->getAs<VectorType>()) {
> > +      if (getContext().getTypeSize(VT) != 128)
> > +        return false;
> >
> > Why not getContext().getTypeSize(VT) <= 128? If I have <3 x float>
> > then this will be expanded to a <3 x float> in the backend, and I
> > don't see why it should not also be passed in the vector registers?
> 
> getTypeSize on a <3 x float> does return 128, it is rounded up to
> the next power of two by common code.  This means that <3 x float>
> is automatically treated as equivalent to <4 x float> for purposes
> of recognizing homogeneous aggregates.  (This is fine, I guess,
> this <3 x float> is a clang language extension type that is neither
> mentioned in the ABI, nor implemented by GCC, so we're free to
> define an ABI for it.)
> 
> A check "getTypeSize <= 128" would also allow <2 x float>, which
> we should *not* do, since that type does exist in GCC, and is
> *not* recognized as homogeneous aggregate there.
> 
> 
> > +  uint32_t NumRegs = Base->isVectorType()? 1 :
> >
> > Put a space before the ?
> 
> Done.
> 
> > I'd really prefer that we use SizeInBits, RegSizeInBits (or just
> > Bits, etc) instead of Size, RegSize, etc. Having sizes both in bits
> > and bytes is a bit confusing ;)
> 
> Agreed.
> 
> > Same here (SizeInBits), and you might as well write 128 as 2*64. In
> > fact, it might be better just to introduce some constant named
> > GPRBits == 64 (or similar) so that it is easier to understand.
> 
> Done as well.
> 
> > -// CHECK: define void @test_struct_v16i16(%struct.v16i16* noalias
> > sret %agg.result, %struct.v16i16* byval align 16)
> > +// CHECK: define void @test_struct_v16i16(%struct.v16i16* noalias
> > sret %agg.result, [2 x i128] %x.coerce)
> >  struct v16i16 test_struct_v16i16(struct v16i16 x)
> >  {
> >    return x;
> >
> > We have a bunch of tests like this, but I don't see any on the
> > caller side. We need to make sure that the coersion works on the
> > caller side. Please add some tests for that.
> >
> > Also, please add a test or two for non-power-of-two vectors (<3 x
> > float> is a good one):
> >   typedef float float3 __attribute__((ext_vector_type(3)));
> 
> Added tests for the caller side and <3 x float> now.
> 
> 
> Since passing non-homogeneous aggregates as array types is now no
> longer a *requirement* on ELFv2, but just an optimization like on
> ELFv1, I've split this patch up in two parts now;  the first piece
> implements ELFv2 support, while the second piece adds the byval
> optimization.
> 
> I've committed the first as revision 213494 and the second as
> revision 213495.
> 
> 
> This should make powerpc64le-linux fully supported in LLVM 3.5.

Great, thanks! Have a good night.

 -Hal

> 
> 
> Bye,
> Ulrich
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory