Page MenuHomePhabricator

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

Authored by erik.pilkington on Jun 3 2019, 3:10 PM.

Details

Summary

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0476r2.html

This adds a new (pseudo) builtin, __builtin_bit_cast(T, v), which performs a bit_cast from a value v to a type T. This expression can be evaluated at compile time under specific circumstances. The compile time evaluation currently doesn't support bit-fields, but I'm planning on fixing this in a follow up (some of the logic for figuring this out is in CodeGen).

rdar://44987528

Thanks for taking a look!

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jfb added inline comments.Jun 3 2019, 5:05 PM
clang/lib/AST/ExprConstant.cpp
5377 ↗(On Diff #202804)

Similar to this, you probably want to assert that CHAR_BIT == whatever we use to denote target bitwidth.

5459 ↗(On Diff #202804)

Does this properly fail when there's a vtable?

rsmith added a comment.Jun 3 2019, 5:10 PM

Can we also use the same bit reader/writer implementation to generalize the current implementation of __builtin_memcpy and friends? (I don't think we can remove the existing implementation, sadly, since we allow copying arrays of the same trivially-copyable type today even if it contains pointers and unions and such.)

clang/include/clang/AST/OperationKinds.def
69–72 ↗(On Diff #202804)

I'm not sure I understand why we don't just use CK_BitCast for this. What's the difference? (The CastKind just specifies how to do the conversion, and doesn't have anything to do with the semantic requirements -- those should be checked by whatever code builds the cast.)

clang/include/clang/Basic/DiagnosticASTKinds.td
219 ↗(On Diff #202804)

Typo: in -> is

clang/include/clang/Basic/DiagnosticSemaKinds.td
9715–9718 ↗(On Diff #202804)

If these diagnostics are going to be produced by instantiations of std::bit_cast, it'd be more user-friendly to avoid mentioning __builtin_bit_cast at all (we'll presumably have a diagnostic pointing at it). So maybe s/__builtin_bit_cast/bit cast/?

clang/include/clang/Basic/Features.def
252 ↗(On Diff #202804)

Should we identify this with __has_builtin instead of __has_extension? (We already list some of our keyword-shaped __builtin_*s there.)

clang/include/clang/Parse/Parser.h
3062–3064 ↗(On Diff #202804)

Please put this somewhere better in the class definition rather than at the end. Maybe with the other named cast expression parsing?

clang/lib/AST/Expr.cpp
3454–3456 ↗(On Diff #202804)

Maybe just add this as another case to the list of *CastExprClass cases above? If we're going to make this a CastExpr, we should at least get some benefit from that... :)

clang/lib/AST/ExprClassification.cpp
423–424 ↗(On Diff #202804)

Likewise here, list this with the other CastExprs.

clang/lib/AST/ExprConstant.cpp
5359 ↗(On Diff #202804)

This only works if unsigned char is large enough to hold one CharUnit, which is theoretically not guaranteed to be the case. Maybe we can defer handling that until we support bit-fields here (or beyond; this is far from the only place in Clang that's not yet ready for char being anything other than 8 bits) but we should at least have a FIXME.

5421–5423 ↗(On Diff #202804)

APValue::None means "outside lifetime", not "uninitialized".

5434–5436 ↗(On Diff #202804)

We should at some point support complex, vector, and fixed point here. Please add a FIXME.

5447 ↗(On Diff #202804)

This will permit bitcasts from nullptr_t, providing a zero value. I think that's wrong; the bit representation of nullptr_t is notionally uninitialized (despite strangely having pointer size and alignment).

I think this is a specification bug: bit_cast<intptr_t>(nullptr) should be non-constant (it won't necessarily be 0 at runtime) but the current wording doesn't seem to permit us to treat it as non-constant.

5486 ↗(On Diff #202804)

Please don't use a literal 8 to encode the target char width; use Info.Context.getCharWidth() instead.

5489 ↗(On Diff #202804)

Use Context.toCharUnitsFromBits here.

5505 ↗(On Diff #202804)

Expanding the array seems unnecessary (and isn't const-correct and might be modifying an actually-const temporary); it doesn't seem hard to use the array filler from here for elements past the last initialized one.

5657 ↗(On Diff #202804)

You support reading atomic types but not writing them. Is that intentional?

6197–6204 ↗(On Diff #202804)

Assuming we switch to using CK_BitCast for all forms of bitcast, we should produce a CCEDiag if the cast expression is not a BuiltinBitCastExpr here.

clang/lib/Sema/SemaCast.cpp
355 ↗(On Diff #202804)

This if condition is redundant with the corresponding check in CheckBitCast.

2801–2802 ↗(On Diff #202804)

Should we be performing the default lvalue conversions here too? Or maybe materializing a temporary? (Are we expecting a glvalue or prvalue as the operand of bit_cast? It seems unnecessarily complex for the AST representation to allow either.)

2805 ↗(On Diff #202804)

isValueDependent is irrelevant here, since you're not going to constant-evaluate SrcExpr.

2831 ↗(On Diff #202804)

If SrcType and DestType are the same (other than cv-qualifiers), we should use CK_NoOp here.

clang/lib/Sema/SemaExceptionSpec.cpp
1207 ↗(On Diff #202804)

List this with the other casts.

clang/lib/Sema/TreeTransform.h
10259–10260 ↗(On Diff #202804)

Please add a RebuildBuiltinBitCastExpr function to TreeTransform and call it here, following the pattern used for other Expr subclasses.

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
1292 ↗(On Diff #202804)

This should be listed with the other CastExprs.

clang/test/CodeGenCXX/builtin-bit-cast.cpp
8–15 ↗(On Diff #202804)

This seems like an unnecessarily-complex lowering. Bitcasting via memory seems fine, but we don't need two separate allocas here -- using only one would seem preferable (especially at -O0), we just need to make sure we don't emit any TBAA metadata for one side of the load/store pair. (Maybe in practice getting CodeGen to do that is too hard and it's not worth it though?)

jfb added inline comments.Jun 3 2019, 5:17 PM
clang/lib/AST/ExprConstant.cpp
5447 ↗(On Diff #202804)

You brought this up in ABQ, the outcome was... poor notes so I have no idea what Core decided...

rsmith added inline comments.Jun 3 2019, 6:08 PM
clang/lib/AST/ExprConstant.cpp
5447 ↗(On Diff #202804)

It looks like we never actually resolved what happens in this case ;-(

5549 ↗(On Diff #202804)

Support for bitcasting to nullptr_t seems to be missing.

jfb added inline comments.Jun 3 2019, 6:56 PM
clang/lib/AST/ExprConstant.cpp
5447 ↗(On Diff #202804)

IIRC we discussed having the same-ish issue with bool, or any other indeterminate bit pattern. Realistically it's zero...

In what sense is the bit-pattern of a null pointer indeterminate? It's implementation-specified, but the compiler is certainly required to be able to produce that value during the constant initialization phase. If the language committee wants to make this non-constant out of some sort of portability concern, that's its business, but it's certainly implementable even if the target has non-zero null pointers or even different null pointer patterns for different types (though of course nullptr_t uses void*'s pattern).

In what sense is the bit-pattern of a null pointer indeterminate?

The problem is not null pointers, it's nullptr_t, which is required to have the same size and alignment as void* but which comprises only padding bits. (Loads of nullptr_t are not even permitted to touch memory...).

In what sense is the bit-pattern of a null pointer indeterminate?

The problem is not null pointers, it's nullptr_t, which is required to have the same size and alignment as void* but which comprises only padding bits. (Loads of nullptr_t are not even permitted to touch memory...).

I mean, I know this is C++ and the committee loves tying itself in knots to make the language unnecessarily unusable, but surely the semantics of bitcasting an r-value of type nullptr_t are intended to be equivalent to bitcasting an r-value of type void* that happens to be a null pointer.

In what sense is the bit-pattern of a null pointer indeterminate?

The problem is not null pointers, it's nullptr_t, which is required to have the same size and alignment as void* but which comprises only padding bits. (Loads of nullptr_t are not even permitted to touch memory...).

I mean, I know this is C++ and the committee loves tying itself in knots to make the language unnecessarily unusable, but surely the semantics of bitcasting an r-value of type nullptr_t are intended to be equivalent to bitcasting an r-value of type void* that happens to be a null pointer.

I don't follow -- why would they be? bit_cast reads the object representation, which for nullptr_t is likely to be uninitialized, because the type contains only padding bits. (Note that there is formally no such thing as "bitcasting an rvalue". bit_cast takes an lvalue, and reinterprets its storage.)

nullptr_t is modeled roughly as if:

struct alignas(void*) nullptr_t {
  template<typename T> operator T*() { return nullptr; }
}

and as with that struct type,bit_cast of nullptr_t to some other type should (presumably) result in an uninitialized value.

(You might argue that it's ridiculous to require that nullptr_t have the same size and alignment as void* but not have the same storage representation as a null void*. I'd agree, and I've raised this in committee before, but without success)

jfb added a comment.Jun 13 2019, 12:15 PM

(You might argue that it's ridiculous to require that nullptr_t have the same size and alignment as void* but not have the same storage representation as a null void*. I'd agree, and I've raised this in committee before, but without success)

We could open a DR for bit_cast<void*>(nullptr_t{}), with proposed resolution that the void* returned is a null void*. Approaching the same problem from a different angle to confuse the prey.

erik.pilkington marked 46 inline comments as done.

Address review comments.

Can we also use the same bit reader/writer implementation to generalize the current implementation of __builtin_memcpy and friends? (I don't think we can remove the existing implementation, sadly, since we allow copying arrays of the same trivially-copyable type today even if it contains pointers and unions and such.)

Yeah, I think we could support that. Sounds like follow-up stuff though :)

clang/include/clang/AST/OperationKinds.def
69–72 ↗(On Diff #202804)

Sure, new patch just folds this into CK_BitCast.

clang/include/clang/Basic/DiagnosticSemaKinds.td
9715–9718 ↗(On Diff #202804)

An invalid __builtin_bit_cast should never really be created, since std::bit_cast guards this with a SFINAE check, so I think its more appropriate to use the __builtin spelling. I just included these diagnostics for completeness, I don't think anyone would actually run into them.

9718 ↗(On Diff #202804)

Sure, "does not match" -> "does not equal".

clang/include/clang/Basic/Features.def
252 ↗(On Diff #202804)

Sure, that makes more sense.

clang/lib/AST/ExprConstant.cpp
5359 ↗(On Diff #202804)

Sure, I added a FIXME.

5364 ↗(On Diff #202804)

Why? Doesn't seem like it would be any more clear, and all the other endian values we're comparing against are bools, which would mean awkwardly translating from an bool -> enum -> bool whenever we want to use it. (i.e. llvm::sys::IsLittleEndianHost, TargetInfo::isLittleEndian() are both booleans)

5377 ↗(On Diff #202804)

Sure, I added an assert down in handleBitCast (which has access to that information).

5421–5423 ↗(On Diff #202804)

New patch updates the diagnostic.

5447 ↗(On Diff #202804)

The new patch writing anything (including an indeterminate value) to nullptr_t, and reading nullptr_t results in indeterminate bytes.

5459 ↗(On Diff #202804)

A virtual function would mean that the record isn't trivially copyable, so that case will be handled in Sema, or even in the SFINAE check. Added a test.

5505 ↗(On Diff #202804)

Yeah, good point. I use the filler in the new patch.

5657 ↗(On Diff #202804)

I haven't considered it until now, but its kinda a moot point, _Atomic types aren't trivially copyable, so a BuiltinBitCastExpr over an atomic type will never be created.

clang/lib/Sema/SemaCast.cpp
2801–2802 ↗(On Diff #202804)

Okay, new patch filters the expression through DefaultLvalueConversion (which deals with placeholders too).

clang/test/CodeGenCXX/builtin-bit-cast.cpp
8–15 ↗(On Diff #202804)

The reason I did the double alloca was TBAA concerns, but it looks like convincing CodeGen to suppress it isn't that hard, so the new patch just does that. We also need to use the max(alignof(dest), alignof(src)) for the alloca.

In what sense is the bit-pattern of a null pointer indeterminate?

The problem is not null pointers, it's nullptr_t, which is required to have the same size and alignment as void* but which comprises only padding bits. (Loads of nullptr_t are not even permitted to touch memory...).

I mean, I know this is C++ and the committee loves tying itself in knots to make the language unnecessarily unusable, but surely the semantics of bitcasting an r-value of type nullptr_t are intended to be equivalent to bitcasting an r-value of type void* that happens to be a null pointer.

I don't follow -- why would they be? bit_cast reads the object representation, which for nullptr_t is likely to be uninitialized, because the type contains only padding bits. (Note that there is formally no such thing as "bitcasting an rvalue". bit_cast takes an lvalue, and reinterprets its storage.)

I agree that the problem is that the object representation of nullptr_t is wrong, but it seems absurd to me that we're going to bake in an absurd special case (from the user's perspective) to bit_cast because we consider that representation unfixable.

rsmith added inline comments.Jun 13 2019, 3:03 PM
clang/lib/AST/ExprConstant.cpp
5360 ↗(On Diff #204609)

A static_assert(std::numeric_limits<unsigned char>::digits >= 8); would be nice.

5461–5469 ↗(On Diff #204609)

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.)

5761–5764 ↗(On Diff #204609)

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.

7098–7099 ↗(On Diff #204609)

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.)

clang/lib/Sema/SemaCast.cpp
2801–2802 ↗(On Diff #202804)

Hmm, sorry, on reflection I think I've led you down the wrong path here.

I think we really do want the operand of the bitcast here to be a glvalue. In the case where the operand is of class type, it will be an lvalue even after DefaultLvalueConversion, because there's no such thing as a CK_LValueToRValue conversion on class type in C++. (For example, this is why you currently need the code in BitCastReader that deals with APValue::LValue -- when bitcasting a class type, you get an lvalue denoting the object rather than the object itself.)

Also, forming an lvalue-to-rvalue conversion here is at least theoretically wrong, because that conversion (notionally) discards the values of padding bits in the "from" object, whereas std::bit_cast requires those values to be preserved into the destination if they are defined in the source object.

Perhaps we should also have different cast kinds for an lvalue-to-rvalue bitcast (this new thing) versus an rvalue-to-rvalue bitcast (CK_BitCast).

clang/test/CodeGenCXX/builtin-bit-cast.cpp
100–112 ↗(On Diff #204609)

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.)

clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
1 ↗(On Diff #204609)

Please add a test that structs with reference members are rejected, along with the other cases from [bit.cast]/2.4-2.8.

In what sense is the bit-pattern of a null pointer indeterminate?

The problem is not null pointers, it's nullptr_t, which is required to have the same size and alignment as void* but which comprises only padding bits. (Loads of nullptr_t are not even permitted to touch memory...).

I mean, I know this is C++ and the committee loves tying itself in knots to make the language unnecessarily unusable, but surely the semantics of bitcasting an r-value of type nullptr_t are intended to be equivalent to bitcasting an r-value of type void* that happens to be a null pointer.

I don't follow -- why would they be? bit_cast reads the object representation, which for nullptr_t is likely to be uninitialized, because the type contains only padding bits. (Note that there is formally no such thing as "bitcasting an rvalue". bit_cast takes an lvalue, and reinterprets its storage.)

I agree that the problem is that the object representation of nullptr_t is wrong, but it seems absurd to me that we're going to bake in an absurd special case (from the user's perspective) to bit_cast because we consider that representation unfixable.

Note that bit_cast supports casting not just from fundamental types but also from aggregates (which might contain a nullptr_t). At runtime, we're going to memcpy from the source object, which will leave the bits in the destination corresponding to nullptr_t subobjects in the source uninitialized (if they were in fact uninitialized in the source object, which they might be). It would be unreasonable to initialize the bits in the bit_cast result during compile-time evaluation but not during runtime evaluation -- compile-time evaluation is supposed to diagnose cases that would be undefined at runtime, not define them.

In my view, the mistake was specifying nullptr_t to have the same size and alignment as void*; it should instead be an empty type. Only confusion results from making it "look like" a pointer type rather than just being an empty tag type.

In what sense is the bit-pattern of a null pointer indeterminate?

The problem is not null pointers, it's nullptr_t, which is required to have the same size and alignment as void* but which comprises only padding bits. (Loads of nullptr_t are not even permitted to touch memory...).

I mean, I know this is C++ and the committee loves tying itself in knots to make the language unnecessarily unusable, but surely the semantics of bitcasting an r-value of type nullptr_t are intended to be equivalent to bitcasting an r-value of type void* that happens to be a null pointer.

I don't follow -- why would they be? bit_cast reads the object representation, which for nullptr_t is likely to be uninitialized, because the type contains only padding bits. (Note that there is formally no such thing as "bitcasting an rvalue". bit_cast takes an lvalue, and reinterprets its storage.)

I agree that the problem is that the object representation of nullptr_t is wrong, but it seems absurd to me that we're going to bake in an absurd special case (from the user's perspective) to bit_cast because we consider that representation unfixable.

Note that bit_cast supports casting not just from fundamental types but also from aggregates (which might contain a nullptr_t). At runtime, we're going to memcpy from the source object, which will leave the bits in the destination corresponding to nullptr_t subobjects in the source uninitialized (if they were in fact uninitialized in the source object, which they might be). It would be unreasonable to initialize the bits in the bit_cast result during compile-time evaluation but not during runtime evaluation -- compile-time evaluation is supposed to diagnose cases that would be undefined at runtime, not define them.

In my view, the mistake was specifying nullptr_t to have the same size and alignment as void*; it should instead be an empty type. Only confusion results from making it "look like" a pointer type rather than just being an empty tag type.

Perhaps, but that's clearly unfixable without breaking ABI, whereas the object-representation issue is fixable by, at most, requiring a few stores that might be difficult to eliminate in some fanciful situations.

In my view, the mistake was specifying nullptr_t to have the same size and alignment as void*; it should instead be an empty type. Only confusion results from making it "look like" a pointer type rather than just being an empty tag type.

Perhaps, but that's clearly unfixable without breaking ABI, whereas the object-representation issue is fixable by, at most, requiring a few stores that might be difficult to eliminate in some fanciful situations.

Requiring initialization of, or assignment to, an object of type nullptr_t to actually store a null pointer value is also an ABI break. (Eg, void f() { decltype(nullptr) n; g(&n); } does not initialize n today in GCC, Clang, or EDG, but would need to do so under the new rule.)

In any case, I've started a discussion on the core reflector. We'll see where that goes.

erik.pilkington marked 16 inline comments as done.

Address review comments, thanks!

clang/lib/AST/ExprConstant.cpp
5461–5469 ↗(On Diff #204609)

Okay, the new patch moves the read to handleBitCast. I added an unreachable here.

5761–5764 ↗(On Diff #204609)

Oh, it looks like the old patch crashed in this case! Turns out structs with reference members are trivially copyable.

7098–7099 ↗(On Diff #204609)

The new patch creates a special cast kind for this, so no changes here are necessary here.

clang/test/CodeGenCXX/builtin-bit-cast.cpp
100–112 ↗(On Diff #204609)

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.

rsmith added inline comments.Jun 21 2019, 2:15 PM
clang/include/clang/Basic/DiagnosticASTKinds.td
223 ↗(On Diff #205713)

"constexpr evaluate" doesn't really mean anything. Also, the "struct with a reference member type X" case seems a bit strangely phrased (and it need not be a struct anyway...).

Maybe "cannot bit_cast {from|to}0 a {|type with a}1 {union|pointer|member pointer|volatile|reference}2 {type|member}1 in a constant expression"?

clang/include/clang/Basic/DiagnosticSemaKinds.td
9732 ↗(On Diff #205713)

We would usually use vs rather than and here.

clang/lib/AST/ExprConstant.cpp
5454–5457 ↗(On Diff #205713)

I've checked the intent on the reflectors, and we should drop both this and the APValue::None check here. Bits corresponding to uninitialized or out-of-lifetime subobjects should just be left uninitialized (exactly like padding bits).

Example:

struct X { char c; int n; };
struct Y { char data[sizeof(X)]; };
constexpr bool test() {
  X x = {1, 2};
  Y y = __builtin_bit_cast(Y, x); // OK, y.data[1] - y.data[3] are APValue::Indeterminate
  X z = __builtin_bit_cast(X, y); // OK, both members are initialized
  return x.c == z.c && x.n == z.n;
}
static_assert(test());
5614–5615 ↗(On Diff #205713)

This should fail with a diagnostic if T is not a byte-like type (an unsigned narrow character type or std::byte), due to [basic.indet]p2.

clang/lib/CodeGen/CGExprComplex.cpp
472–475 ↗(On Diff #205713)

This seems to unnecessarily force an extra round-trip through memory. Op is an lvalue, so it already describes a value in memory, and you should be able to use EmitLValue(Op) to get the lvalue you want to load from.

clang/lib/CodeGen/CGExprScalar.cpp
2045–2048 ↗(On Diff #205713)

Likewise here.

Quuxplusone added inline comments.
clang/include/clang/Basic/DiagnosticASTKinds.td
223 ↗(On Diff #205713)

Peanut gallery says: Surely wherever this message comes from should use the same wording as other similar messages: "(this construction) is not allowed in a constant expression". That is, the diagnostics for

constexpr int *foo(void *p) { return reinterpret_cast<int*>(p); }
constexpr int *bar(void *p) { return std::bit_cast<int*>(p); }

should be word-for-word identical except for the kind of cast they're complaining about.

(And if I had my pedantic druthers, every such message would say "...does not produce a constant expression" instead of "...is not allowed in a constant expression." But that's way out of scope for this patch.)

erik.pilkington marked 10 inline comments as done.

Address review comments.

clang/include/clang/Basic/DiagnosticASTKinds.td
223 ↗(On Diff #205713)

Sure, that seems more consistent.

clang/lib/AST/ExprConstant.cpp
5454–5457 ↗(On Diff #205713)

You mean:

- struct Y { char data[sizeof(X)]; };
+ struct Y { unsigned char data[sizeof(X)]; };

...right? I added this test to the constexpr-builtin-bit-cast.cpp (as well of some of your other cases from the reflector).

rsmith accepted this revision.Jul 1 2019, 11:24 AM

LG with a few tweaks.

clang/include/clang/Basic/DiagnosticASTKinds.td
228–230 ↗(On Diff #206304)

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.

clang/lib/AST/ExprConstant.cpp
5380 ↗(On Diff #206304)

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?

5430 ↗(On Diff #206304)

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?

clang/lib/CodeGen/CGExprComplex.cpp
474–476 ↗(On Diff #206304)

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.

clang/lib/CodeGen/CGExprScalar.cpp
2043–2045 ↗(On Diff #206304)

Likewise here, I think we should not be changing the alignment.

clang/test/CodeGenCXX/builtin-bit-cast.cpp
18–20 ↗(On Diff #206304)

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.)

clang/test/SemaCXX/builtin-bit-cast.cpp
35 ↗(On Diff #206304)

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.)

clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
30 ↗(On Diff #206304)

Return bool here rather than int.

230 ↗(On Diff #206304)

Please also test bitcasting from uninitialized and out of lifetime objects to nullptr_t.

This revision is now accepted and ready to land.Jul 1 2019, 11:24 AM
This revision was automatically updated to reflect the committed changes.