<div dir="ltr">That was certainly one of the counterarguments (& global variables also use a type rather than a size). I've not really settled on which way to go/haven't given it lots of thought. I may loop back around to the original thread when it comes to that.</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 10, 2016 at 2:18 PM, James Y Knight <span dir="ltr"><<a href="mailto:jyknight@google.com" target="_blank">jyknight@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Yes, it was intended to -- at least for bitcode produced by clang.<div><br></div><div>I do think it would be a good idea to continue to pass the value type to byval, though...Either that or get rid of the type in the "alloca" instruction. They're basically doing the same thing, and having them specified completely differently seems unfortunate.</div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 10, 2016 at 4:58 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Excuse the necromancy, but do you know if this change (or other work you did in this area) completely eclipsed LLVM's use of inferred alignment via the llvm struct's alignment for byval arguments?<br><br>I ask because this was something I was going to need to fix for the typeless pointer work & I have some outdated patches, etc, that I can throw away if that's the case (at least it looks like it touched most of the places my patch did).<br><br>(I'll still need to remove the pointer type in favor of a number of bytes for byval (byval to copy (or moving the type from the argument's pointer type, into a parameter to byval, eg: %ptr byval(struct_foo) %arg) but knowing the alignment's totally handled would be a good first step in that work)</div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 21, 2015 at 11:19 AM, James Y Knight via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: jyknight<br>
Date: Fri Aug 21 13:19:06 2015<br>
New Revision: 245719<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=245719&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=245719&view=rev</a><br>
Log:<br>
Properly provide alignment of 'byval' arguments down to llvm.<br>
<br>
This is important in the case that the LLVM-inferred llvm-struct<br>
alignment is not the same as the clang-known C-struct alignment.<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D12243" rel="noreferrer" target="_blank">http://reviews.llvm.org/D12243</a><br>
<br>
Added:<br>
cfe/trunk/test/CodeGen/sparc-arguments.c<br>
Modified:<br>
cfe/trunk/lib/CodeGen/CGCall.cpp<br>
cfe/trunk/test/CodeGen/le32-arguments.c<br>
cfe/trunk/test/CodeGen/nvptx-abi.c<br>
<br>
Modified: cfe/trunk/lib/CodeGen/CGCall.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=245719&r1=245718&r2=245719&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=245719&r1=245718&r2=245719&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/CodeGen/CGCall.cpp (original)<br>
+++ cfe/trunk/lib/CodeGen/CGCall.cpp Fri Aug 21 13:19:06 2015<br>
@@ -1663,20 +1663,35 @@ void CodeGenModule::ConstructAttributeLi<br>
Attrs.addAttribute(llvm::Attribute::InReg);<br>
break;<br>
<br>
- case ABIArgInfo::Indirect:<br>
+ case ABIArgInfo::Indirect: {<br>
if (AI.getInReg())<br>
Attrs.addAttribute(llvm::Attribute::InReg);<br>
<br>
if (AI.getIndirectByVal())<br>
Attrs.addAttribute(llvm::Attribute::ByVal);<br>
<br>
- Attrs.addAlignmentAttr(AI.getIndirectAlign());<br>
+ unsigned Align = AI.getIndirectAlign();<br>
+<br>
+ // In a byval argument, it is important that the required<br>
+ // alignment of the type is honored, as LLVM might be creating a<br>
+ // *new* stack object, and needs to know what alignment to give<br>
+ // it. (Sometimes it can deduce a sensible alignment on its own,<br>
+ // but not if clang decides it must emit a packed struct, or the<br>
+ // user specifies increased alignment requirements.)<br>
+ //<br>
+ // This is different from indirect *not* byval, where the object<br>
+ // exists already, and the align attribute is purely<br>
+ // informative.<br>
+ if (Align == 0 && AI.getIndirectByVal())<br>
+ Align = getContext().getTypeAlignInChars(ParamType).getQuantity();<br>
+<br>
+ Attrs.addAlignmentAttr(Align);<br>
<br>
// byval disables readnone and readonly.<br>
FuncAttrs.removeAttribute(llvm::Attribute::ReadOnly)<br>
.removeAttribute(llvm::Attribute::ReadNone);<br>
break;<br>
-<br>
+ }<br>
case ABIArgInfo::Ignore:<br>
case ABIArgInfo::Expand:<br>
continue;<br>
<br>
Modified: cfe/trunk/test/CodeGen/le32-arguments.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/le32-arguments.c?rev=245719&r1=245718&r2=245719&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/le32-arguments.c?rev=245719&r1=245718&r2=245719&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/CodeGen/le32-arguments.c (original)<br>
+++ cfe/trunk/test/CodeGen/le32-arguments.c Fri Aug 21 13:19:06 2015<br>
@@ -10,7 +10,7 @@ typedef struct {<br>
int bb;<br>
} s1;<br>
// Structs should be passed byval and not split up<br>
-// CHECK-LABEL: define void @f1(%struct.s1* byval %i)<br>
+// CHECK-LABEL: define void @f1(%struct.s1* byval align 4 %i)<br>
void f1(s1 i) {}<br>
<br>
typedef struct {<br>
@@ -48,7 +48,7 @@ union simple_union {<br>
char b;<br>
};<br>
// Unions should be passed as byval structs<br>
-// CHECK-LABEL: define void @f7(%union.simple_union* byval %s)<br>
+// CHECK-LABEL: define void @f7(%union.simple_union* byval align 4 %s)<br>
void f7(union simple_union s) {}<br>
<br>
typedef struct {<br>
@@ -57,5 +57,5 @@ typedef struct {<br>
int b8 : 8;<br>
} bitfield1;<br>
// Bitfields should be passed as byval structs<br>
-// CHECK-LABEL: define void @f8(%struct.bitfield1* byval %bf1)<br>
+// CHECK-LABEL: define void @f8(%struct.bitfield1* byval align 4 %bf1)<br>
void f8(bitfield1 bf1) {}<br>
<br>
Modified: cfe/trunk/test/CodeGen/nvptx-abi.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/nvptx-abi.c?rev=245719&r1=245718&r2=245719&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/nvptx-abi.c?rev=245719&r1=245718&r2=245719&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/CodeGen/nvptx-abi.c (original)<br>
+++ cfe/trunk/test/CodeGen/nvptx-abi.c Fri Aug 21 13:19:06 2015<br>
@@ -21,14 +21,14 @@ float bar(void) {<br>
<br>
void foo(float4_t x) {<br>
// CHECK-LABEL: @foo<br>
-// CHECK: %struct.float4_s* byval %x<br>
+// CHECK: %struct.float4_s* byval align 4 %x<br>
}<br>
<br>
void fooN(float4_t x, float4_t y, float4_t z) {<br>
// CHECK-LABEL: @fooN<br>
-// CHECK: %struct.float4_s* byval %x<br>
-// CHECK: %struct.float4_s* byval %y<br>
-// CHECK: %struct.float4_s* byval %z<br>
+// CHECK: %struct.float4_s* byval align 4 %x<br>
+// CHECK: %struct.float4_s* byval align 4 %y<br>
+// CHECK: %struct.float4_s* byval align 4 %z<br>
}<br>
<br>
typedef struct nested_s {<br>
@@ -39,5 +39,5 @@ typedef struct nested_s {<br>
<br>
void baz(nested_t x) {<br>
// CHECK-LABEL: @baz<br>
-// CHECK: %struct.nested_s* byval %x)<br>
+// CHECK: %struct.nested_s* byval align 8 %x)<br>
}<br>
<br>
Added: cfe/trunk/test/CodeGen/sparc-arguments.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/sparc-arguments.c?rev=245719&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/sparc-arguments.c?rev=245719&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/CodeGen/sparc-arguments.c (added)<br>
+++ cfe/trunk/test/CodeGen/sparc-arguments.c Fri Aug 21 13:19:06 2015<br>
@@ -0,0 +1,27 @@<br>
+// RUN: %clang_cc1 -triple sparc-unknown-unknown -emit-llvm -o - %s | FileCheck %s<br>
+<br>
+// Ensure that we pass proper alignment to llvm in the call<br>
+// instruction. The proper alignment for the type is sometimes known<br>
+// only by clang, and is not manifest in the LLVM-type. So, it must be<br>
+// explicitly passed through. (Besides the case of the user specifying<br>
+// alignment, as here, this situation also occurrs for non-POD C++<br>
+// structs with tail-padding: clang emits these as packed llvm-structs<br>
+// for ABI reasons.)<br>
+<br>
+struct s1 {<br>
+ int x;<br>
+} __attribute__((aligned(8)));<br>
+<br>
+struct s1 x1;<br>
+<br>
+<br>
+// Ensure the align 8 is passed through:<br>
+// CHECK-LABEL: define void @f1()<br>
+// CHECK: call void @f1_helper(%struct.s1* byval align 8 @x1)<br>
+// Also ensure the declaration of f1_helper includes it<br>
+// CHECK: declare void @f1_helper(%struct.s1* byval align 8)<br>
+<br>
+void f1_helper(struct s1);<br>
+void f1() {<br>
+ f1_helper(x1);<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>