[llvm] r190636 - Fix PPC ABI for ByVal structs with vector members

Thu Feb 19 16:32:15 PST 2015

On Thu, Feb 19, 2015 at 4:24 PM, Hal Finkel <hfinkel at anl.gov> wrote:

>
>
> ----- Original Message -----
> > From: "David Blaikie" <dblaikie at gmail.com>
> > To: "Hal Finkel" <hfinkel at anl.gov>
> > Cc: llvm-commits at cs.uiuc.edu
> > Sent: Thursday, February 19, 2015 3:57:13 PM
> > Subject: Re: [llvm] r190636 - Fix PPC ABI for ByVal structs with vector
> members
> >
> >
> > On Thu, Sep 12, 2013 at 4:20 PM, Hal Finkel < hfinkel at anl.gov >
> > wrote:
> >
> >
> > Author: hfinkel
> > Date: Thu Sep 12 18:20:06 2013
> > New Revision: 190636
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=190636&view=rev
> > Log:
> > Fix PPC ABI for ByVal structs with vector members
> >
> > When a structure is passed by value, and that structure contains a
> > vector
> > member, according to the PPC ABI, the structure will receive enhanced
> > alignment
> > (so that the vector within the structure will always be aligned).
> >
> > This should resolve PR16641.
> >
> >
> > In an effort to simplify byval in a future world of opaque (untyped)
> > pointers, I was considering changing "%struct.foo byval" to "ptr
> > byval(num_bytes)" - which I think would work for every target except
> > PPC (PPC, and the code in this commit, is the only case I could find
> > of overriding getByValTypeAlignment that wasn't just the default of
> > asking DataLayout for the type's alignment).
> >
> > Is this code necessary? If I remove the entire PPCTargetLowering
> > override of getByValTypeAlignment this test case (& everything else
> > in check-llvm) appears to still pass.
> >
>
> Interesting. I'll need to look at this in more detail. I'm fairly certain
> that it was needed when it was added. It may also still be necessary for
> QPX support (which is incomplete in tree at the moment, but that should
> change in the next few days). In short, please give me a few days, and then
> I should have a better idea. Theoretically, however, I'm pretty sure it is
> necessary.
>

Sure, no worries.

One theory I have is that changes that've been made since then have lead to
the frontend providing the right alignment via align attributes on byval
parameters.

If this logic is required & the align provided by clang (or other
frontends?) is insufficient for these interesting rules, we might want to
move this logic up into clang. (so that LLVM's byval support doesn't need a
type - just a size and alignment of bytes to copy)

- David

>
>  -Hal
>
> >
> >
> > Added:
> > llvm/trunk/test/CodeGen/PowerPC/vec-abi-align.ll
> > Modified:
> > llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp
> >
> > Modified: llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp
> > URL:
> >
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp?rev=190636&r1=190635&r2=190636&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp (original)
> > +++ llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp Thu Sep 12
> > 18:20:06 2013
> > @@ -578,24 +578,48 @@ PPCTargetLowering::PPCTargetLowering(PPC
> > }
> > }
> >
> > +/// getMaxByValAlign - Helper for getByValTypeAlignment to determine
> > +/// the desired ByVal argument alignment.
> > +static void getMaxByValAlign(Type *Ty, unsigned &MaxAlign,
> > + unsigned MaxMaxAlign) {
> > + if (MaxAlign == MaxMaxAlign)
> > + return;
> > + if (VectorType *VTy = dyn_cast<VectorType>(Ty)) {
> > + if (MaxMaxAlign >= 32 && VTy->getBitWidth() >= 256)
> > + MaxAlign = 32;
> > + else if (VTy->getBitWidth() >= 128 && MaxAlign < 16)
> > + MaxAlign = 16;
> > + } else if (ArrayType *ATy = dyn_cast<ArrayType>(Ty)) {
> > + unsigned EltAlign = 0;
> > + getMaxByValAlign(ATy->getElementType(), EltAlign, MaxMaxAlign);
> > + if (EltAlign > MaxAlign)
> > + MaxAlign = EltAlign;
> > + } else if (StructType *STy = dyn_cast<StructType>(Ty)) {
> > + for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {
> > + unsigned EltAlign = 0;
> > + getMaxByValAlign(STy->getElementType(i), EltAlign, MaxMaxAlign);
> > + if (EltAlign > MaxAlign)
> > + MaxAlign = EltAlign;
> > + if (MaxAlign == MaxMaxAlign)
> > + break;
> > + }
> > + }
> > +}
> > +
> > /// getByValTypeAlignment - Return the desired alignment for ByVal
> > aggregate
> > /// function arguments in the caller parameter area.
> > unsigned PPCTargetLowering::getByValTypeAlignment(Type *Ty) const {
> > const TargetMachine &TM = getTargetMachine();
> > // Darwin passes everything on 4 byte boundary.
> > - if (TM.getSubtarget<PPCSubtarget>().isDarwin())
> > + if (PPCSubTarget.isDarwin())
> > return 4;
> >
> > // 16byte and wider vectors are passed on 16byte boundary.
> > - if (VectorType *VTy = dyn_cast<VectorType>(Ty))
> > - if (VTy->getBitWidth() >= 128)
> > - return 16;
> > -
> > // The rest is 8 on PPC64 and 4 on PPC32 boundary.
> > - if (PPCSubTarget.isPPC64())
> > - return 8;
> > -
> > - return 4;
> > + unsigned Align = PPCSubTarget.isPPC64() ? 8 : 4;
> > + if (PPCSubTarget.hasAltivec() || PPCSubTarget.hasQPX())
> > + getMaxByValAlign(Ty, Align, PPCSubTarget.hasQPX() ? 32 : 16);
> > + return Align;
> > }
> >
> > const char *PPCTargetLowering::getTargetNodeName(unsigned Opcode)
> > const {
> > @@ -2281,6 +2305,13 @@ PPCTargetLowering::LowerFormalArguments_
> > InVals.push_back(FIN);
> > continue;
> > }
> > +
> > + unsigned BVAlign = Flags.getByValAlign();
> > + if (BVAlign > 8) {
> > + ArgOffset = ((ArgOffset+BVAlign-1)/BVAlign)*BVAlign;
> > + CurArgOffset = ArgOffset;
> > + }
> > +
> > // All aggregates smaller than 8 bytes must be passed
> > right-justified.
> > if (ObjSize < PtrByteSize)
> > CurArgOffset = CurArgOffset + (PtrByteSize - ObjSize);
> > @@ -3870,6 +3901,15 @@ PPCTargetLowering::LowerCall_64SVR4(SDVa
> > if (Size == 0)
> > continue;
> >
> > + unsigned BVAlign = Flags.getByValAlign();
> > + if (BVAlign > 8) {
> > + if (BVAlign % PtrByteSize != 0)
> > + llvm_unreachable(
> > + "ByVal alignment is not a multiple of the pointer size");
> > +
> > + ArgOffset = ((ArgOffset+BVAlign-1)/BVAlign)*BVAlign;
> > + }
> > +
> > // All aggregates smaller than 8 bytes must be passed
> > right-justified.
> > if (Size==1 || Size==2 || Size==4) {
> > EVT VT = (Size==1) ? MVT::i8 : ((Size==2) ? MVT::i16 : MVT::i32);
> >
> > Added: llvm/trunk/test/CodeGen/PowerPC/vec-abi-align.ll
> > URL:
> >
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/PowerPC/vec-abi-align.ll?rev=190636&view=auto
> >
> ==============================================================================
> > --- llvm/trunk/test/CodeGen/PowerPC/vec-abi-align.ll (added)
> > +++ llvm/trunk/test/CodeGen/PowerPC/vec-abi-align.ll Thu Sep 12
> > 18:20:06 2013
> > @@ -0,0 +1,64 @@
> > +; RUN: llc -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7 < %s |
> > FileCheck %s
> > +target datalayout =
> >
> "E-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-f128:128:128-v128:128:128-n32:64"
> > +target triple = "powerpc64-unknown-linux-gnu"
> > +
> > +%struct.s2 = type { i64, <4 x float> }
> > +
> > + at ve = external global <4 x float>
> > + at n = external global i64
> > +
> > +; Function Attrs: nounwind
> > +define void @test1(i64 %d1, i64 %d2, i64 %d3, i64 %d4, i64 %d5, i64
> > %d6, i64 %d7, i64 %d8, i64 %d9, <4 x float> inreg %vs.coerce) #0 {
> > +entry:
> > + store <4 x float> %vs.coerce, <4 x float>* @ve, align 16, !tbaa !0
> > + ret void
> > +
> > +; CHECK-LABEL: @test1
> > +; CHECK: stvx 2,
> > +; CHECK: blr
> > +}
> > +
> > +; Function Attrs: nounwind
> > +define void @test2(i64 %d1, i64 %d2, i64 %d3, i64 %d4, i64 %d5, i64
> > %d6, i64 %d7, i64 %d8, %struct.s2* byval nocapture readonly %vs) #0
> > {
> > +entry:
> > + %m = getelementptr inbounds %struct.s2* %vs, i64 0, i32 0
> > + %0 = load i64* %m, align 8, !tbaa !2
> > + store i64 %0, i64* @n, align 8, !tbaa !2
> > + %v = getelementptr inbounds %struct.s2* %vs, i64 0, i32 1
> > + %1 = load <4 x float>* %v, align 16, !tbaa !0
> > + store <4 x float> %1, <4 x float>* @ve, align 16, !tbaa !0
> > + ret void
> > +
> > +; CHECK-LABEL: @test2
> > +; CHECK: ld {{[0-9]+}}, 112(1)
> > +; CHECK: li [[REG16:[0-9]+]], 16
> > +; CHECK: addi [[REGB:[0-9]+]], 1, 112
> > +; CHECK: lvx 2, [[REGB]], [[REG16]]
> > +; CHECK: blr
> > +}
> > +
> > +; Function Attrs: nounwind
> > +define void @test3(i64 %d1, i64 %d2, i64 %d3, i64 %d4, i64 %d5, i64
> > %d6, i64 %d7, i64 %d8, i64 %d9, %struct.s2* byval nocapture readonly
> > %vs) #0 {
> > +entry:
> > + %m = getelementptr inbounds %struct.s2* %vs, i64 0, i32 0
> > + %0 = load i64* %m, align 8, !tbaa !2
> > + store i64 %0, i64* @n, align 8, !tbaa !2
> > + %v = getelementptr inbounds %struct.s2* %vs, i64 0, i32 1
> > + %1 = load <4 x float>* %v, align 16, !tbaa !0
> > + store <4 x float> %1, <4 x float>* @ve, align 16, !tbaa !0
> > + ret void
> > +
> > +; CHECK-LABEL: @test3
> > +; CHECK: ld {{[0-9]+}}, 128(1)
> > +; CHECK: li [[REG16:[0-9]+]], 16
> > +; CHECK: addi [[REGB:[0-9]+]], 1, 128
> > +; CHECK: lvx 2, [[REGB]], [[REG16]]
> > +; CHECK: blr
> > +}
> > +
> > +attributes #0 = { nounwind }
> > +
> > +!0 = metadata !{metadata !"omnipotent char", metadata !1}
> > +!1 = metadata !{metadata !"Simple C/C++ TBAA"}
> > +!2 = metadata !{metadata !"long", metadata !0}
> > +
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150219/fde37c4d/attachment.html>