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

[Erik Pilkington via Phabricator via llvm-commits]

erik.pilkington added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:5461-5469
+    case APValue::LValue: {
+      LValue LVal;
+      LVal.setFrom(Info.Ctx, Val);
+      APValue RVal;
+      if (!handleLValueToRValueConversion(Info, BCE, Ty.withConst(),
+                                          LVal, RVal))
+        return false;
----------------
rsmith wrote:
> This looks wrong: bitcasting a pointer should not dereference the pointer and store its pointee! (Likewise for a reference member.) But I think this should actually simply be unreachable: we already checked for pointers and reference members in `checkBitCastConstexprEligibilityType`.
> 
> (However, I think it currently *is* reached, because there's also some cases where the operand of the bitcast is an lvalue, and the resulting lvalue gets misinterpreted as a value of the underlying type, landing us here. See below.)
Okay, the new patch moves the read to handleBitCast. I added an unreachable here. 


================
Comment at: clang/lib/AST/ExprConstant.cpp:5761-5764
+    for (FieldDecl *FD : Record->fields()) {
+      if (!checkBitCastConstexprEligibilityType(Loc, FD->getType(), Info, Ctx))
+        return note(0, FD->getType(), FD->getBeginLoc());
+    }
----------------
rsmith wrote:
> The spec for `bit_cast` also asks us to check for members of reference type. That can't happen because a type with reference members can never be trivially-copyable, but please include an assert here to demonstrate to a reader that we've thought about and handled that case.
Oh, it looks like the old patch crashed in this case! Turns out structs with reference members are trivially copyable.


================
Comment at: clang/lib/AST/ExprConstant.cpp:7098-7099
   case CK_BitCast:
+    // CK_BitCast originating from a __builtin_bit_cast have different constexpr
+    // semantics. This is only reachable when bit casting to nullptr_t.
+    if (isa<BuiltinBitCastExpr>(E))
----------------
rsmith wrote:
> I think this is reversed from the way I'd think about it: casts to `void*` are modeled as bit-casts, and so need special handling here. (Theoretically it'd be preferable to call the base class function for all other kinds of bitcast we encounter, but it turns out to not matter because only bitcasts to `nullptr_t` are ever evaluatable currently. In future we might want to support bit-casts between integers and pointers (for constant folding only), and then it might matter.)
The new patch creates a special cast kind for this, so no changes here are necessary here.


================
Comment at: clang/test/CodeGenCXX/builtin-bit-cast.cpp:100-112
+void test_array(int (&ary)[2]) {
+  // CHECK-LABEL: define void @_Z10test_arrayRA2_i
+  __builtin_bit_cast(unsigned long, ary);
+
+  // CHECK: [[ARY_PTR:%.*]] = alloca [2 x i32]*, align 8
+  // CHECK-NEXT: [[TEMP]] = alloca [2 x i32], align 8
+  // store ary into ARY_PTR
----------------
rsmith wrote:
> If we remove the `DefaultLvalueConversion` from building the bitcast expression, we should be able to avoid the unnecessary extra alloca here, and instead `memcpy` directly from `ary` to the `unsigned long`. (This is the most important testcase in this file, since it's the only one that gives `__builtin_bit_cast` an lvalue operand, which is what `std::bit_cast` will do.)
`ScalarExprEmitter::VisitCastExpr` has to return a Value of type `i32` here though, so we don't know where to memcpy the array to when emitting the cast. I guess we could bitcast the pointer and load from it if the alignments are compatible, and fall back to a temporary otherwise (such as this case, where alignof(ary) is 4, but alignof(unsigned long) is 8). I think its more in the spirit of clang CodeGen to just emit simple code that always works though, then leave any optimizations to, well, the optimizer (assuming the `-O0` output isn't too horrible). We can use this CodeGen strategy for aggregate destinations, which the new patch does.

> This is the most important testcase in this file, since it's the only one that gives __builtin_bit_cast an lvalue operand

Yeah, good point. New patch gives lvalue operands to the other calls.


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

https://reviews.llvm.org/D62825