[PATCH] D24378: [CodeGen] Provide an appropriate alignment for dynamic allocas

[David Majnemer via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL285316: [CodeGen] Provide an appropriate alignment for 
dynamic allocas (authored by majnemer).

Changed prior to commit:
  https://reviews.llvm.org/D24378?vs=70789=76060#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D24378

Files:
  cfe/trunk/lib/CodeGen/CGBuiltin.cpp
  cfe/trunk/test/CodeGen/builtins-ms.c


Index: cfe/trunk/test/CodeGen/builtins-ms.c
===
--- cfe/trunk/test/CodeGen/builtins-ms.c
+++ cfe/trunk/test/CodeGen/builtins-ms.c
@@ -4,6 +4,6 @@
 void capture(void *);
 void test_alloca(int n) {
   capture(_alloca(n));
-  // CHECK: %[[arg:.*]] = alloca i8, i32 %
+  // CHECK: %[[arg:.*]] = alloca i8, i32 %{{.*}}, align 16
   // CHECK: call void @capture(i8* %[[arg]])
 }
Index: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
===
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp
@@ -1139,7 +1139,13 @@
   case Builtin::BI_alloca:
   case Builtin::BI__builtin_alloca: {
 Value *Size = EmitScalarExpr(E->getArg(0));
-return RValue::get(Builder.CreateAlloca(Builder.getInt8Ty(), Size));
+const TargetInfo  = getContext().getTargetInfo();
+// The alignment of the alloca should correspond to __BIGGEST_ALIGNMENT__.
+unsigned SuitableAlignmentInBytes =
+TI.getSuitableAlign() / TI.getCharWidth();
+AllocaInst *AI = Builder.CreateAlloca(Builder.getInt8Ty(), Size);
+AI->setAlignment(SuitableAlignmentInBytes);
+return RValue::get(AI);
   }
   case Builtin::BIbzero:
   case Builtin::BI__builtin_bzero: {


Index: cfe/trunk/test/CodeGen/builtins-ms.c
===
--- cfe/trunk/test/CodeGen/builtins-ms.c
+++ cfe/trunk/test/CodeGen/builtins-ms.c
@@ -4,6 +4,6 @@
 void capture(void *);
 void test_alloca(int n) {
   capture(_alloca(n));
-  // CHECK: %[[arg:.*]] = alloca i8, i32 %
+  // CHECK: %[[arg:.*]] = alloca i8, i32 %{{.*}}, align 16
   // CHECK: call void @capture(i8* %[[arg]])
 }
Index: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
===
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp
@@ -1139,7 +1139,13 @@
   case Builtin::BI_alloca:
   case Builtin::BI__builtin_alloca: {
 Value *Size = EmitScalarExpr(E->getArg(0));
-return RValue::get(Builder.CreateAlloca(Builder.getInt8Ty(), Size));
+const TargetInfo  = getContext().getTargetInfo();
+// The alignment of the alloca should correspond to __BIGGEST_ALIGNMENT__.
+unsigned SuitableAlignmentInBytes =
+TI.getSuitableAlign() / TI.getCharWidth();
+AllocaInst *AI = Builder.CreateAlloca(Builder.getInt8Ty(), Size);
+AI->setAlignment(SuitableAlignmentInBytes);
+return RValue::get(AI);
   }
   case Builtin::BIbzero:
   case Builtin::BI__builtin_bzero: {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24378: [CodeGen] Provide an appropriate alignment for dynamic allocas

[Eli Friedman via cfe-commits]

efriedma added a comment.

Ping


https://reviews.llvm.org/D24378



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24378: [CodeGen] Provide an appropriate alignment for dynamic allocas

[Richard Smith via cfe-commits]

On Fri, Sep 9, 2016 at 10:45 AM, Friedman, Eli via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> It probably makes sense to say that alloca should have the same alignment
> as the default stack alignment, simply because it's hard to do anything
> useful with completely unaligned memory.  That said, the default stack
> alignment is 4 bytes on 32-bit Windows, not 16.
>

Well, if the allocated size is dynamic, it would seem preferable to provide
a pointer that's suitably-aligned for anything that can fit within it, and
even on 32-bit x86 that means 16 byte alignment if we want to support SSE.
But if the size is known, there doesn't seem to be any need to provide an
alignment greater than the allocated size.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24378: [CodeGen] Provide an appropriate alignment for dynamic allocas

[Eli Friedman via cfe-commits]

efriedma added a comment.

Huh... then I guess this is fine.


https://reviews.llvm.org/D24378



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24378: [CodeGen] Provide an appropriate alignment for dynamic allocas

[Reid Kleckner via cfe-commits]

rnk added a comment.

In https://reviews.llvm.org/D24378#538430, @efriedma wrote:

> This is probably going to lead to someone complaining about clang realigning 
> the stack on 32-bit Windows; are your sure we're choosing the right alignment 
> there?


I checked, and MSVC emits a call to __alloca_probe_16 for this code:

  extern "C" void *_alloca(size_t);
  #pragma intrinsic (_alloca)
  void g(void*);
  void f(int n) {
void *p = _alloca(n);
g(p);
  }

So, they do realign the stack. We don't really need to realign the whole stack 
frame for highly aligned dynamic allocas, though. There's no reason we can't 
overallocate from the stack and realign the returned pointer. I'm not sure if 
we do that, though...


https://reviews.llvm.org/D24378



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24378: [CodeGen] Provide an appropriate alignment for dynamic allocas

[Friedman, Eli via cfe-commits]

It probably makes sense to say that alloca should have the same 
alignment as the default stack alignment, simply because it's hard to do 
anything useful with completely unaligned memory.  That said, the 
default stack alignment is 4 bytes on 32-bit Windows, not 16.


-Eli

On 9/9/2016 10:29 AM, Richard Smith wrote:
There's an (unconvincing to me) explanation for why gcc does this 
here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19131


On 9 Sep 2016 10:14 am, "Eli Friedman" > wrote:


efriedma added a comment.

This is probably going to lead to someone complaining about clang
realigning the stack on 32-bit Windows; are your sure we're
choosing the right alignment there?


https://reviews.llvm.org/D24378 




--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24378: [CodeGen] Provide an appropriate alignment for dynamic allocas

[Richard Smith via cfe-commits]

There's an (unconvincing to me) explanation for why gcc does this here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19131

On 9 Sep 2016 10:14 am, "Eli Friedman"  wrote:

> efriedma added a comment.
>
> This is probably going to lead to someone complaining about clang
> realigning the stack on 32-bit Windows; are your sure we're choosing the
> right alignment there?
>
>
> https://reviews.llvm.org/D24378
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24378: [CodeGen] Provide an appropriate alignment for dynamic allocas

[Eli Friedman via cfe-commits]

efriedma added a comment.

This is probably going to lead to someone complaining about clang realigning 
the stack on 32-bit Windows; are your sure we're choosing the right alignment 
there?


https://reviews.llvm.org/D24378



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24378: [CodeGen] Provide an appropriate alignment for dynamic allocas

[Reid Kleckner via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm


https://reviews.llvm.org/D24378



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24378: [CodeGen] Provide an appropriate alignment for dynamic allocas

[David Majnemer via cfe-commits]

majnemer created this revision.
majnemer added reviewers: rnk, rsmith, efriedma, chandlerc.
majnemer added a subscriber: cfe-commits.

GCC documents __builtin_alloca as aligning the storage to at least
__BIGGEST_ALIGNMENT__.

MSVC documents essentially the same for the x64 ABI:
https://msdn.microsoft.com/en-us/library/x9sx5da1.aspx

The 32-bit ABI follows the same rule: it emits a call to _alloca_probe_16

https://reviews.llvm.org/D24378

Files:
  lib/CodeGen/CGBuiltin.cpp
  test/CodeGen/builtins-ms.c

Index: test/CodeGen/builtins-ms.c
===
--- test/CodeGen/builtins-ms.c
+++ test/CodeGen/builtins-ms.c
@@ -4,6 +4,6 @@
 void capture(void *);
 void test_alloca(int n) {
   capture(_alloca(n));
-  // CHECK: %[[arg:.*]] = alloca i8, i32 %
+  // CHECK: %[[arg:.*]] = alloca i8, i32 %{{.*}}, align 16
   // CHECK: call void @capture(i8* %[[arg]])
 }
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -1015,7 +1015,13 @@
   case Builtin::BI_alloca:
   case Builtin::BI__builtin_alloca: {
 Value *Size = EmitScalarExpr(E->getArg(0));
-return RValue::get(Builder.CreateAlloca(Builder.getInt8Ty(), Size));
+const TargetInfo  = getContext().getTargetInfo();
+// The alignment of the alloca should correspond to __BIGGEST_ALIGNMENT__.
+unsigned SuitableAlignmentInBytes =
+TI.getSuitableAlign() / TI.getCharWidth();
+AllocaInst *AI = Builder.CreateAlloca(Builder.getInt8Ty(), Size);
+AI->setAlignment(SuitableAlignmentInBytes);
+return RValue::get(AI);
   }
   case Builtin::BIbzero:
   case Builtin::BI__builtin_bzero: {


Index: test/CodeGen/builtins-ms.c
===
--- test/CodeGen/builtins-ms.c
+++ test/CodeGen/builtins-ms.c
@@ -4,6 +4,6 @@
 void capture(void *);
 void test_alloca(int n) {
   capture(_alloca(n));
-  // CHECK: %[[arg:.*]] = alloca i8, i32 %
+  // CHECK: %[[arg:.*]] = alloca i8, i32 %{{.*}}, align 16
   // CHECK: call void @capture(i8* %[[arg]])
 }
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -1015,7 +1015,13 @@
   case Builtin::BI_alloca:
   case Builtin::BI__builtin_alloca: {
 Value *Size = EmitScalarExpr(E->getArg(0));
-return RValue::get(Builder.CreateAlloca(Builder.getInt8Ty(), Size));
+const TargetInfo  = getContext().getTargetInfo();
+// The alignment of the alloca should correspond to __BIGGEST_ALIGNMENT__.
+unsigned SuitableAlignmentInBytes =
+TI.getSuitableAlign() / TI.getCharWidth();
+AllocaInst *AI = Builder.CreateAlloca(Builder.getInt8Ty(), Size);
+AI->setAlignment(SuitableAlignmentInBytes);
+return RValue::get(AI);
   }
   case Builtin::BIbzero:
   case Builtin::BI__builtin_bzero: {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits