<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 19, 2015 at 4:24 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
<br>
----- Original Message -----<br>
> From: "David Blaikie" <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
> Cc: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> Sent: Thursday, February 19, 2015 3:57:13 PM<br>
> Subject: Re: [llvm] r190636 - Fix PPC ABI for ByVal structs with vector members<br>
><br>
><br>
> On Thu, Sep 12, 2013 at 4:20 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> wrote:<br>
><br>
><br>
> Author: hfinkel<br>
> Date: Thu Sep 12 18:20:06 2013<br>
> New Revision: 190636<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=190636&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=190636&view=rev</a><br>
> Log:<br>
> Fix PPC ABI for ByVal structs with vector members<br>
><br>
> When a structure is passed by value, and that structure contains a<br>
> vector<br>
> member, according to the PPC ABI, the structure will receive enhanced<br>
> alignment<br>
> (so that the vector within the structure will always be aligned).<br>
><br>
> This should resolve PR16641.<br>
><br>
><br>
> In an effort to simplify byval in a future world of opaque (untyped)<br>
> pointers, I was considering changing "%struct.foo byval" to "ptr<br>
> byval(num_bytes)" - which I think would work for every target except<br>
> PPC (PPC, and the code in this commit, is the only case I could find<br>
> of overriding getByValTypeAlignment that wasn't just the default of<br>
> asking DataLayout for the type's alignment).<br>
><br>
> Is this code necessary? If I remove the entire PPCTargetLowering<br>
> override of getByValTypeAlignment this test case (& everything else<br>
> in check-llvm) appears to still pass.<br>
><br>
<br>
</div></div>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.<br></blockquote><div><br>Sure, no worries.<br><br>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.<br><br>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)<br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
 -Hal<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> Added:<br>
> llvm/trunk/test/CodeGen/PowerPC/vec-abi-align.ll<br>
> Modified:<br>
> llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp<br>
><br>
> Modified: llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp?rev=190636&r1=190635&r2=190636&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp?rev=190636&r1=190635&r2=190636&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp (original)<br>
> +++ llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp Thu Sep 12<br>
> 18:20:06 2013<br>
> @@ -578,24 +578,48 @@ PPCTargetLowering::PPCTargetLowering(PPC<br>
> }<br>
> }<br>
><br>
> +/// getMaxByValAlign - Helper for getByValTypeAlignment to determine<br>
> +/// the desired ByVal argument alignment.<br>
> +static void getMaxByValAlign(Type *Ty, unsigned &MaxAlign,<br>
> + unsigned MaxMaxAlign) {<br>
> + if (MaxAlign == MaxMaxAlign)<br>
> + return;<br>
> + if (VectorType *VTy = dyn_cast<VectorType>(Ty)) {<br>
> + if (MaxMaxAlign >= 32 && VTy->getBitWidth() >= 256)<br>
> + MaxAlign = 32;<br>
> + else if (VTy->getBitWidth() >= 128 && MaxAlign < 16)<br>
> + MaxAlign = 16;<br>
> + } else if (ArrayType *ATy = dyn_cast<ArrayType>(Ty)) {<br>
> + unsigned EltAlign = 0;<br>
> + getMaxByValAlign(ATy->getElementType(), EltAlign, MaxMaxAlign);<br>
> + if (EltAlign > MaxAlign)<br>
> + MaxAlign = EltAlign;<br>
> + } else if (StructType *STy = dyn_cast<StructType>(Ty)) {<br>
> + for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {<br>
> + unsigned EltAlign = 0;<br>
> + getMaxByValAlign(STy->getElementType(i), EltAlign, MaxMaxAlign);<br>
> + if (EltAlign > MaxAlign)<br>
> + MaxAlign = EltAlign;<br>
> + if (MaxAlign == MaxMaxAlign)<br>
> + break;<br>
> + }<br>
> + }<br>
> +}<br>
> +<br>
> /// getByValTypeAlignment - Return the desired alignment for ByVal<br>
> aggregate<br>
> /// function arguments in the caller parameter area.<br>
> unsigned PPCTargetLowering::getByValTypeAlignment(Type *Ty) const {<br>
> const TargetMachine &TM = getTargetMachine();<br>
> // Darwin passes everything on 4 byte boundary.<br>
> - if (TM.getSubtarget<PPCSubtarget>().isDarwin())<br>
> + if (PPCSubTarget.isDarwin())<br>
> return 4;<br>
><br>
> // 16byte and wider vectors are passed on 16byte boundary.<br>
> - if (VectorType *VTy = dyn_cast<VectorType>(Ty))<br>
> - if (VTy->getBitWidth() >= 128)<br>
> - return 16;<br>
> -<br>
> // The rest is 8 on PPC64 and 4 on PPC32 boundary.<br>
> - if (PPCSubTarget.isPPC64())<br>
> - return 8;<br>
> -<br>
> - return 4;<br>
> + unsigned Align = PPCSubTarget.isPPC64() ? 8 : 4;<br>
> + if (PPCSubTarget.hasAltivec() || PPCSubTarget.hasQPX())<br>
> + getMaxByValAlign(Ty, Align, PPCSubTarget.hasQPX() ? 32 : 16);<br>
> + return Align;<br>
> }<br>
><br>
> const char *PPCTargetLowering::getTargetNodeName(unsigned Opcode)<br>
> const {<br>
> @@ -2281,6 +2305,13 @@ PPCTargetLowering::LowerFormalArguments_<br>
> InVals.push_back(FIN);<br>
> continue;<br>
> }<br>
> +<br>
> + unsigned BVAlign = Flags.getByValAlign();<br>
> + if (BVAlign > 8) {<br>
> + ArgOffset = ((ArgOffset+BVAlign-1)/BVAlign)*BVAlign;<br>
> + CurArgOffset = ArgOffset;<br>
> + }<br>
> +<br>
> // All aggregates smaller than 8 bytes must be passed<br>
> right-justified.<br>
> if (ObjSize < PtrByteSize)<br>
> CurArgOffset = CurArgOffset + (PtrByteSize - ObjSize);<br>
> @@ -3870,6 +3901,15 @@ PPCTargetLowering::LowerCall_64SVR4(SDVa<br>
> if (Size == 0)<br>
> continue;<br>
><br>
> + unsigned BVAlign = Flags.getByValAlign();<br>
> + if (BVAlign > 8) {<br>
> + if (BVAlign % PtrByteSize != 0)<br>
> + llvm_unreachable(<br>
> + "ByVal alignment is not a multiple of the pointer size");<br>
> +<br>
> + ArgOffset = ((ArgOffset+BVAlign-1)/BVAlign)*BVAlign;<br>
> + }<br>
> +<br>
> // All aggregates smaller than 8 bytes must be passed<br>
> right-justified.<br>
> if (Size==1 || Size==2 || Size==4) {<br>
> EVT VT = (Size==1) ? MVT::i8 : ((Size==2) ? MVT::i16 : MVT::i32);<br>
><br>
> Added: llvm/trunk/test/CodeGen/PowerPC/vec-abi-align.ll<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/PowerPC/vec-abi-align.ll?rev=190636&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/PowerPC/vec-abi-align.ll?rev=190636&view=auto</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/CodeGen/PowerPC/vec-abi-align.ll (added)<br>
> +++ llvm/trunk/test/CodeGen/PowerPC/vec-abi-align.ll Thu Sep 12<br>
> 18:20:06 2013<br>
> @@ -0,0 +1,64 @@<br>
> +; RUN: llc -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7 < %s |<br>
> FileCheck %s<br>
> +target datalayout =<br>
> "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"<br>
> +target triple = "powerpc64-unknown-linux-gnu"<br>
> +<br>
> +%struct.s2 = type { i64, <4 x float> }<br>
> +<br>
> +@ve = external global <4 x float><br>
> +@n = external global i64<br>
> +<br>
> +; Function Attrs: nounwind<br>
> +define void @test1(i64 %d1, i64 %d2, i64 %d3, i64 %d4, i64 %d5, i64<br>
> %d6, i64 %d7, i64 %d8, i64 %d9, <4 x float> inreg %vs.coerce) #0 {<br>
> +entry:<br>
> + store <4 x float> %vs.coerce, <4 x float>* @ve, align 16, !tbaa !0<br>
> + ret void<br>
> +<br>
> +; CHECK-LABEL: @test1<br>
> +; CHECK: stvx 2,<br>
> +; CHECK: blr<br>
> +}<br>
> +<br>
> +; Function Attrs: nounwind<br>
> +define void @test2(i64 %d1, i64 %d2, i64 %d3, i64 %d4, i64 %d5, i64<br>
> %d6, i64 %d7, i64 %d8, %struct.s2* byval nocapture readonly %vs) #0<br>
> {<br>
> +entry:<br>
> + %m = getelementptr inbounds %struct.s2* %vs, i64 0, i32 0<br>
> + %0 = load i64* %m, align 8, !tbaa !2<br>
> + store i64 %0, i64* @n, align 8, !tbaa !2<br>
> + %v = getelementptr inbounds %struct.s2* %vs, i64 0, i32 1<br>
> + %1 = load <4 x float>* %v, align 16, !tbaa !0<br>
> + store <4 x float> %1, <4 x float>* @ve, align 16, !tbaa !0<br>
> + ret void<br>
> +<br>
> +; CHECK-LABEL: @test2<br>
> +; CHECK: ld {{[0-9]+}}, 112(1)<br>
> +; CHECK: li [[REG16:[0-9]+]], 16<br>
> +; CHECK: addi [[REGB:[0-9]+]], 1, 112<br>
> +; CHECK: lvx 2, [[REGB]], [[REG16]]<br>
> +; CHECK: blr<br>
> +}<br>
> +<br>
> +; Function Attrs: nounwind<br>
> +define void @test3(i64 %d1, i64 %d2, i64 %d3, i64 %d4, i64 %d5, i64<br>
> %d6, i64 %d7, i64 %d8, i64 %d9, %struct.s2* byval nocapture readonly<br>
> %vs) #0 {<br>
> +entry:<br>
> + %m = getelementptr inbounds %struct.s2* %vs, i64 0, i32 0<br>
> + %0 = load i64* %m, align 8, !tbaa !2<br>
> + store i64 %0, i64* @n, align 8, !tbaa !2<br>
> + %v = getelementptr inbounds %struct.s2* %vs, i64 0, i32 1<br>
> + %1 = load <4 x float>* %v, align 16, !tbaa !0<br>
> + store <4 x float> %1, <4 x float>* @ve, align 16, !tbaa !0<br>
> + ret void<br>
> +<br>
> +; CHECK-LABEL: @test3<br>
> +; CHECK: ld {{[0-9]+}}, 128(1)<br>
> +; CHECK: li [[REG16:[0-9]+]], 16<br>
> +; CHECK: addi [[REGB:[0-9]+]], 1, 128<br>
> +; CHECK: lvx 2, [[REGB]], [[REG16]]<br>
> +; CHECK: blr<br>
> +}<br>
> +<br>
> +attributes #0 = { nounwind }<br>
> +<br>
> +!0 = metadata !{metadata !"omnipotent char", metadata !1}<br>
> +!1 = metadata !{metadata !"Simple C/C++ TBAA"}<br>
> +!2 = metadata !{metadata !"long", metadata !0}<br>
> +<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div></div>