[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

LG with a few tweaks.



================
Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:228-230
+def note_constexpr_bit_cast_indet_dest : Note<
+  "indeterminate value can only initialize an object of type 'unsigned char' "
+  "or 'std::byte'; %0 is invalid">;
----------------
This is incorrect; you can also bitcast to `char` if it's unsigned (under `-funsigned-char`). Use `getLangOpts().CharIsSigned` to detect whether we're in that mode.


================
Comment at: clang/lib/AST/ExprConstant.cpp:5380
 
+class APBuffer {
+  // FIXME: We're going to need bit-level granularity when we support
----------------
I don't think there's really anything "arbitrary-precision" about this `APBuffer`. (It's a bad name for `APValue` and a worse name here.) Maybe `BitCastBuffer` would be better?


================
Comment at: clang/lib/AST/ExprConstant.cpp:5430
+/// would represent the value at runtime.
+class BitCastReader {
+  EvalInfo &Info;
----------------
Every time I come back to this patch I find these class names confusing -- the reader writes to the buffer, and the writer reads from it. I think more explicit names would help: `APValueToAPBufferConverter` and `APBufferToAPValueConverter` maybe?


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:474-476
+    CharUnits Align = std::min(Ctx.getTypeAlignInChars(Op->getType()),
+                               Ctx.getTypeAlignInChars(DestTy));
+    DestLV.setAlignment(Align);
----------------
Do we need to change the alignment here at all? The alignment on `DestLV` should already be taken from `Addr` (whose alignment in turn is taken from `SourceLVal`), and should be the alignment computed when emitting the source lvalue expression, which seems like the right alignment to use for the load. The natural alignment of the destination type (or the source type for that matter) doesn't seem relevant.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2043-2045
+    CharUnits Align = std::min(Ctx.getTypeAlignInChars(E->getType()),
+                               Ctx.getTypeAlignInChars(DestTy));
+    DestLV.setAlignment(Align);
----------------
Likewise here, I think we should not be changing the alignment.


================
Comment at: clang/test/CodeGenCXX/builtin-bit-cast.cpp:18-20
+void test_aggregate_to_scalar(two_ints &ti) {
+  // CHECK-LABEL: define void @_Z24test_aggregate_to_scalarR8two_ints
+  __builtin_bit_cast(unsigned long, ti);
----------------
Change the return type to `unsigned long` and `return __builtin_bit_cast(...)` so that future improvements to discarded value expression lowering don't invalidate your test. (That'll also better mirror the expected use in `std::bit_cast`.)


================
Comment at: clang/test/SemaCXX/builtin-bit-cast.cpp:35
+// expected-error at +1{{__builtin_bit_cast source type must be trivially copyable}}
+constexpr unsigned long ul = __builtin_bit_cast(unsigned long, not_trivially_copyable{});
----------------
Please also explicitly test `__builtin_bit_cast` to a reference type. (Reference types aren't trivially-copyable, but this seems like an important enough special case to be worth calling out in the tests -- usually, casting to a reference is the same thing as casting the address of the operand to a pointer and dereferencing, but not here.)


================
Comment at: clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp:30
+template <class Intermediate, class Init>
+constexpr int round_trip(const Init &init) {
+  return bit_cast<Init>(bit_cast<Intermediate>(init)) == init;
----------------
Return `bool` here rather than `int`.


================
Comment at: clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp:230
+constexpr int test_to_nullptr() {
+  nullptr_t npt = __builtin_bit_cast(nullptr_t, 0ul);
+  void *ptr = npt;
----------------
Please also test bitcasting from uninitialized and out of lifetime objects to `nullptr_t`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62825/new/

https://reviews.llvm.org/D62825