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