[PATCH] D25581: Implement __builtin_alloca_with_align for GCC compatibility

2016-11-30 Thread Reid Kleckner via Phabricator via cfe-commits
rnk abandoned this revision.
rnk added a subscriber: majnemer.
rnk added a comment.

@majnemer did this in https://reviews.llvm.org/rL285544


https://reviews.llvm.org/D25581



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


[PATCH] D25581: Implement __builtin_alloca_with_align for GCC compatibility

2016-10-13 Thread Richard Smith via cfe-commits
rsmith added inline comments.



Comment at: lib/CodeGen/CGBuiltin.cpp:1037-1039
+llvm::APSInt AlignInBits;
+if (!E->getArg(1)->EvaluateAsInt(AlignInBits, CGM.getContext()))
+  break;

This takes the alignment in **bits**? That's so ridiculously dumb that I would 
feel bad about accepting this patch unless it comes with a warning for people 
writing the obvious-but-wrong `__builtin_alloca_with_align(sizeof(T), 
alignof(T))`.



Comment at: lib/CodeGen/CGBuiltin.cpp:1038-1039
+llvm::APSInt AlignInBits;
+if (!E->getArg(1)->EvaluateAsInt(AlignInBits, CGM.getContext()))
+  break;
+unsigned Align =

`EvaluateKnownConstInt`, perhaps?



Comment at: lib/CodeGen/CGBuiltin.cpp:1040-1041
+  break;
+unsigned Align =
+AlignInBits.getZExtValue() / CGM.getContext().getCharWidth();
+return RValue::get(

`CGM.getContext().toCharUnitsFromBits` maybe?



Comment at: lib/Sema/SemaChecking.cpp:971-972
+llvm::APSInt AlignAP;
+bool AlignIsConst = TheCall->getArg(1)->EvaluateAsInt(AlignAP, Context);
+assert(AlignIsConst && "basic checking failed");
+// Keep this in sync with llvm::Value::MaximumAlignment.

`EvaluateKnownConstInt`, perhaps?



Comment at: lib/Sema/SemaChecking.cpp:973
+assert(AlignIsConst && "basic checking failed");
+// Keep this in sync with llvm::Value::MaximumAlignment.
+unsigned MaxAlignBytes = 1U << 29;

Well, this comment won't cause that to happen. Do you anticipate that value 
changing?



Comment at: test/Sema/builtin-alloca.c:7
+  __builtin_alloca_with_align(n, 4); // expected-error {{requested bit 
alignment is not a multiple of CHAR_WIDTH}}
+  __builtin_alloca_with_align(n, 8ULL<<30); // expected-error {{requested 
alignment must be 536870912 bytes or smaller}}
+  __builtin_alloca_with_align(n, 8);

Can you also add tests for `-1` and `(__int128)1 << 100`? (They should both 
pass, already, but seem like interesting corner cases regardless.)


https://reviews.llvm.org/D25581



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


[PATCH] D25581: Implement __builtin_alloca_with_align for GCC compatibility

2016-10-13 Thread Reid Kleckner via cfe-commits
rnk created this revision.
rnk added a reviewer: rsmith.
rnk added a subscriber: cfe-commits.

GCC documents that the alignment parameter is in bits and it must be:

- an integer constant
- a power of two
- a multiple of CHAR_BITS
- below an unspecified limit

This lines up directly with the requirements for LLVM's alloca
instruction, so I went ahead and wired it up. We limit the alignment to
1<<29, which is the maximum alignment supported by LLVM's alloca.

Implements feature request in PR30658


https://reviews.llvm.org/D25581

Files:
  include/clang/Basic/Builtins.def
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/CodeGen/CGBuiltin.cpp
  lib/Sema/SemaChecking.cpp
  test/CodeGen/alloca.c
  test/Sema/builtin-alloca.c

Index: test/Sema/builtin-alloca.c
===
--- /dev/null
+++ test/Sema/builtin-alloca.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 %s -verify -triple=x86_64-linux
+
+void aligned_alloca(__SIZE_TYPE__ n) {
+  __builtin_alloca_with_align(n, n); // expected-error {{argument to '__builtin_alloca_with_align' must be a constant integer}}
+  __builtin_alloca_with_align(n, 3); // expected-error {{requested alignment is not a power of 2}}
+  __builtin_alloca_with_align(n, 4); // expected-error {{requested bit alignment is not a multiple of CHAR_WIDTH}}
+  __builtin_alloca_with_align(n, 8ULL<<30); // expected-error {{requested alignment must be 536870912 bytes or smaller}}
+  __builtin_alloca_with_align(n, 8);
+  __builtin_alloca_with_align(n, 64);
+  __builtin_alloca_with_align(n, 128);
+  __builtin_alloca_with_align(n, 4096);
+}
Index: test/CodeGen/alloca.c
===
--- test/CodeGen/alloca.c
+++ test/CodeGen/alloca.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -emit-llvm %s  -o /dev/null
+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm %s -o - | FileCheck %s
 
 typedef __SIZE_TYPE__ size_t;
 void *alloca(size_t size);
@@ -9,3 +9,21 @@
   strcpy(C, argv[0]);
   puts(C);
 }
+
+// CHECK-LABEL: define i32 @main
+// CHECK: alloca i8, i64 %{{.*}}
+// CHECK: call i8* @strcpy
+
+
+void aligned_alloca(size_t n) {
+  __builtin_alloca_with_align(n, 8);
+  __builtin_alloca_with_align(n, 64);
+  __builtin_alloca_with_align(n, 128);
+  __builtin_alloca_with_align(n, 4096);
+}
+
+// CHECK-LABEL: define void @aligned_alloca
+// CHECK: alloca i8, i64 %{{[^,]*}}, align 1
+// CHECK: alloca i8, i64 %{{[^,]*}}, align 8
+// CHECK: alloca i8, i64 %{{[^,]*}}, align 16
+// CHECK: alloca i8, i64 %{{[^,]*}}, align 512
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -966,6 +966,28 @@
 DeclareGlobalNewDelete();
 break;
 
+  case Builtin::BI__builtin_alloca_with_align: {
+llvm::APSInt AlignAP;
+bool AlignIsConst = TheCall->getArg(1)->EvaluateAsInt(AlignAP, Context);
+assert(AlignIsConst && "basic checking failed");
+// Keep this in sync with llvm::Value::MaximumAlignment.
+unsigned MaxAlignBytes = 1U << 29;
+if (!AlignAP.isPowerOf2()) {
+  Diag(TheCall->getExprLoc(), diag::err_alignment_not_power_of_two);
+  return ExprError();
+}
+if (AlignAP > MaxAlignBytes * 8ULL) {
+  Diag(TheCall->getExprLoc(), diag::err_attribute_aligned_too_great)
+  << MaxAlignBytes;
+  return ExprError();
+}
+if (AlignAP.getZExtValue() % Context.getCharWidth() != 0) {
+  Diag(TheCall->getExprLoc(), diag::err_alignment_not_char_width);
+  return ExprError();
+}
+break;
+  }
+
   // check secure string manipulation functions where overflows
   // are detectable at compile time
   case Builtin::BI__builtin___memcpy_chk:
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -1027,6 +1027,21 @@
 Value *Size = EmitScalarExpr(E->getArg(0));
 return RValue::get(Builder.CreateAlloca(Builder.getInt8Ty(), Size));
   }
+  case Builtin::BI__builtin_alloca_with_align: {
+// FIXME: The GCC docs say that the "lifetime of the allocated object ends
+// at the end of the block in which the function was called", but "the
+// allocated storage is released no later than just before the calling
+// function returns to its caller". Our implementation behaves like a normal
+// alloca, which releases the allocated memory on function return.
+Value *Size = EmitScalarExpr(E->getArg(0));
+llvm::APSInt AlignInBits;
+if (!E->getArg(1)->EvaluateAsInt(AlignInBits, CGM.getContext()))
+  break;
+unsigned Align =
+AlignInBits.getZExtValue() / CGM.getContext().getCharWidth();
+return RValue::get(
+Builder.Insert(new AllocaInst(Builder.getInt8Ty(), Size, Align)));
+  }
   case Builtin::BIbzero:
   case Builtin::BI__builtin_bzero: {
 Address Dest = EmitPointerWithAlignment(E->getArg(0));
Index: