[PATCH] D25581: Implement __builtin_alloca_with_align for GCC compatibility

[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