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