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

Mon Jun 3 17:10:49 PDT 2019

rsmith added a comment.

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

================
Comment at: clang/include/clang/AST/OperationKinds.def:69-72
+/// CK_CXXBitCast - Represents std::bit_cast. Like CK_BitCast, but with very few
+/// semantic requirements, namely, the source and destination types just need to
+/// be of the same size and trivially copyable. There is no CK_LValueBitCast
+/// analog for this, since std::bit_cast isn't able to create a need for it.
----------------
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.)

================
Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:219
+def note_constexpr_bit_cast_unsupported_bitfield : Note<
+  "constexpr bit_cast involving bit-field in not yet supported">;
+def note_constexpr_bit_cast_invalid_type : Note<
----------------
Typo: in -> is

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9715-9718
+def err_bit_cast_non_trivially_copyable : Error<
+  "__builtin_bit_cast %select{source|destination}0 type must be a trivially copyable">;
+def err_bit_cast_type_size_mismatch : Error<
+  "__builtin_bit_cast source size does not match destination size (%0 and %1)">;
----------------
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/?

================
Comment at: clang/include/clang/Basic/Features.def:252
 EXTENSION(gnu_asm, LangOpts.GNUAsm)
+EXTENSION(builtin_bit_cast, true)

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

================
Comment at: clang/include/clang/Parse/Parser.h:3062-3064
+
+  /// Parse a __builtin_bit_cast(T, E).
+  ExprResult ParseBuiltinBitCast();
----------------
Please put this somewhere better in the class definition rather than at the end. Maybe with the other named cast expression parsing?

================
Comment at: clang/lib/AST/Expr.cpp:3454-3456
+  case BuiltinBitCastExprClass:
+    return cast<BuiltinBitCastExpr>(this)->getSubExpr()->HasSideEffects(
+        Ctx, IncludePossibleEffects);
----------------
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... :)

================
Comment at: clang/lib/AST/ExprClassification.cpp:423-424
     return ClassifyInternal(Ctx, cast<CoroutineSuspendExpr>(E)->getResumeExpr());
+  case Expr::BuiltinBitCastExprClass:
+    return Cl::CL_PRValue;
   }
----------------
Likewise here, list this with the other `CastExpr`s.

================
Comment at: clang/lib/AST/ExprConstant.cpp:5359
+  // bit-fields.
+  SmallVector<Optional<unsigned char>, 32> Bytes;
+
----------------
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.

================
Comment at: clang/lib/AST/ExprConstant.cpp:5421-5423
+      Info.FFDiag(BCE->getBeginLoc(), diag::note_constexpr_uninitialized)
+          << true << Ty;
+      return false;
----------------
`APValue::None` means "outside lifetime", not "uninitialized".

================
Comment at: clang/lib/AST/ExprConstant.cpp:5434-5436
+    case APValue::ComplexInt:
+    case APValue::ComplexFloat:
+    case APValue::Vector:
----------------
We should at some point support complex, vector, and fixed point here. Please add a FIXME.

================
Comment at: clang/lib/AST/ExprConstant.cpp:5447
+
+    case APValue::LValue: {
+      LValue LVal;
----------------
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.

================
Comment at: clang/lib/AST/ExprConstant.cpp:5486
+
+      assert(FieldOffsetBits % 8 == 0 &&
+             "only bit-fields can have sub-char alignment");
----------------
Please don't use a literal `8` to encode the target `char` width; use `Info.Context.getCharWidth()` instead.

================
Comment at: clang/lib/AST/ExprConstant.cpp:5489
+      CharUnits FieldOffset =
+          CharUnits::fromQuantity(FieldOffsetBits / 8) + Offset;
+      QualType FieldTy = FD->getType();
----------------
Use `Context.toCharUnitsFromBits` here.

================
Comment at: clang/lib/AST/ExprConstant.cpp:5505
+    CharUnits ElemWidth = Info.Ctx.getTypeSizeInChars(CAT->getElementType());
+    expandArray(const_cast<APValue &>(Val), Val.getArraySize()-1);
+    for (unsigned I = 0, E = Val.getArraySize(); I != E; ++I) {
----------------
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.

================
Comment at: clang/lib/AST/ExprConstant.cpp:5657
+
+  Optional<APValue> visit(const Type *Ty, CharUnits Offset) {
+    return unsupportedType(QualType(Ty, 0));
----------------
You support reading atomic types but not writing them. Is that intentional?

================
Comment at: clang/lib/AST/ExprConstant.cpp:6197-6204
+    case CK_CXXBitCast: {
+      APValue DestValue, SourceValue;
+      if (!Evaluate(SourceValue, Info, E->getSubExpr()))
+        return false;
+      if (!handleBitCast(Info, DestValue, SourceValue, E))
+        return false;
+      return DerivedSuccess(DestValue, E);
----------------
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.

================
Comment at: clang/lib/Sema/SemaCast.cpp:355
+
+  if (!Operand->isTypeDependent() && !TSI->getType()->isDependentType()) {
+    Op.CheckBitCast();
----------------
This `if` condition is redundant with the corresponding check in `CheckBitCast`.

================
Comment at: clang/lib/Sema/SemaCast.cpp:2801-2802
+void CastOperation::CheckBitCast() {
+  if (isPlaceholder())
+    SrcExpr = Self.CheckPlaceholderExpr(SrcExpr.get());
+
----------------
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.)

================
Comment at: clang/lib/Sema/SemaCast.cpp:2805
+  if (DestType->isDependentType() || SrcExpr.get()->isTypeDependent() ||
+      SrcExpr.get()->isValueDependent())
+    return;
----------------
`isValueDependent` is irrelevant here, since you're not going to constant-evaluate `SrcExpr`.

================
Comment at: clang/lib/Sema/SemaCast.cpp:2831
+
+  Kind = CK_CXXBitCast;
+}
----------------
If `SrcType` and `DestType` are the same (other than cv-qualifiers), we should use `CK_NoOp` here.

================
Comment at: clang/lib/Sema/SemaExceptionSpec.cpp:1207
   case Expr::VAArgExprClass:
+  case Expr::BuiltinBitCastExprClass:
     return canSubExprsThrow(*this, E);
----------------
List this with the other casts.

================
Comment at: clang/lib/Sema/TreeTransform.h:10259-10260
+
+  return getSema().BuildBuiltinBitCastExpr(BCE->getBeginLoc(), TSI, Sub.get(),
+                                           BCE->getEndLoc());
+}
----------------
Please add a `RebuildBuiltinBitCastExpr` function to `TreeTransform` and call it here, following the pattern used for other `Expr` subclasses.

================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:1292
+    case Stmt::CapturedStmtClass:
+    case Stmt::BuiltinBitCastExprClass: {
       const ExplodedNode *node = Bldr.generateSink(S, Pred, Pred->getState());
----------------
This should be listed with the other `CastExpr`s.

================
Comment at: clang/test/CodeGenCXX/builtin-bit-cast.cpp:8-15
+  // CHECK: [[BIT_CAST_TEMP_INT:%.*]] = alloca i32, align 4
+  // CHECK: [[BIT_CAST_TEMP_FLOAT:%.*]] = alloca float, align 4
+
+  // CHECK: store i32 43, i32* [[BIT_CAST_TEMP_INT]], align 4
+  // ...bitcast the memcpy operands to i8*...
+  // CHECK: call void @llvm.memcpy.p0i8.p0i8.i64
+  // CHECK-NEXT: [[TMP:%.*]] = bitcast i8* %1 to float*
----------------
This seems like an unnecessarily-complex lowering. Bitcasting via memory seems fine, but we don't need two separate `alloca`s 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?)

Repository:
  rC Clang

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

https://reviews.llvm.org/D62825