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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
