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

[Ulrich Weigand]

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.


Bye,
Ulrich