----- Original Message ----- > From: "Ulrich Weigand" <[email protected]> > To: "Hal Finkel" <[email protected]> > Cc: "cfe-commits" <[email protected]> > Sent: Sunday, July 20, 2014 8:29:05 PM > Subject: Re: [PATCH] PowerPC support for the ELFv2 ABI (powerpc64le-linux) > > Hal Finkel <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
