[Clang][C++20] Implement constexpr std::bit_cast for bit-fields #74775
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change introspects more values involved in a static_assert, and
extends the supported set of operators for introspection to include
binary operator method calls.
It's intended to address the use-case where a small static_assert helper
looks something like this (via `constexpr-builtin-bit-cast.cpp`):
```c++
struct int_splicer {
unsigned x;
unsigned y;
constexpr bool operator==(const int_splicer &other) const {
return other.x == x && other.y == y;
}
};
```
When used like so:
```c++
constexpr int_splicer got{1, 2};
constexpr int_splicer want{3, 4};
static_assert(got == want);
```
Then we'd expect to get the error:
```
Static assertion failed due to requirement 'got == want'
```
And this change adds the helpful note:
```
Expression evaluates to '{1, 2} == {3, 4}'
```
After this commit, clang permits constructions like:
```c++
struct bits {
unsigned char : 7;
unsigned char flag : 1;
}
static_assert(std::bit_cast<bits>(0x80).flag); // succeeds on little-endian systems
```
This change builds on the prior work in https://reviews.llvm.org/D62825
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (sethp) ChangesAfter this commit, clang permits constructions like: struct bits {
unsigned char : 7;
unsigned char flag : 1;
}
static_assert(std::bit_cast<bits>(0x80).flag); // succeeds on little-endian systemsA few notes for the reviewers:
To better inform the shape of work that I see as outstanding here, I was hoping to solicit your feedback on three specific points:
Thanks for all the hard work y'all do! Patch is 51.99 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/74775.diff 9 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
index c81d17ed64108..7020f70f7c1b0 100644
--- a/clang/include/clang/Basic/DiagnosticASTKinds.td
+++ b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -316,10 +316,14 @@ def note_constexpr_memcpy_unsupported : Note<
"size to copy (%4) is not a multiple of size of element type %3 (%5)|"
"source is not a contiguous array of at least %4 elements of type %3|"
"destination is not a contiguous array of at least %4 elements of type %3}2">;
+def note_constexpr_bit_cast_bad_bits : Note<
+ "bit_cast source expression (type %5) does not produce a constant value for "
+ "%select{bit|byte}0 [%1] (of {%2%plural{0:|:..0}2}) which are required by "
+ "target type %4 %select{|(subobject %3)}6">;
def note_constexpr_bit_cast_unsupported_type : Note<
"constexpr bit cast involving type %0 is not yet supported">;
-def note_constexpr_bit_cast_unsupported_bitfield : Note<
- "constexpr bit_cast involving bit-field is not yet supported">;
+def note_constexpr_bit_cast_invalid_decl : Note<
+ "bit_cast here %select{from|to}0 invalid declaration %0">;
def note_constexpr_bit_cast_invalid_type : Note<
"bit_cast %select{from|to}0 a %select{|type with a }1"
"%select{union|pointer|member pointer|volatile|reference}2 "
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 986302e1fd225..356cef552d354 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -38,7 +38,6 @@
#include "Interp/State.h"
#include "clang/AST/APValue.h"
#include "clang/AST/ASTContext.h"
-#include "clang/AST/ASTDiagnostic.h"
#include "clang/AST/ASTLambda.h"
#include "clang/AST/Attr.h"
#include "clang/AST/CXXInheritance.h"
@@ -49,19 +48,33 @@
#include "clang/AST/OptionalDiagnostic.h"
#include "clang/AST/RecordLayout.h"
#include "clang/AST/StmtVisitor.h"
+#include "clang/AST/Type.h"
#include "clang/AST/TypeLoc.h"
#include "clang/Basic/Builtins.h"
-#include "clang/Basic/DiagnosticSema.h"
+#include "clang/Basic/DiagnosticAST.h"
#include "clang/Basic/TargetInfo.h"
#include "llvm/ADT/APFixedPoint.h"
+#include "llvm/ADT/APInt.h"
+#include "llvm/ADT/APSInt.h"
#include "llvm/ADT/SmallBitVector.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Compiler.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/SaveAndRestore.h"
+#include "llvm/Support/SwapByteOrder.h"
#include "llvm/Support/TimeProfiler.h"
#include "llvm/Support/raw_ostream.h"
+#include <algorithm>
+#include <cstddef>
+#include <cstdint>
#include <cstring>
#include <functional>
+#include <iomanip>
+#include <iterator>
#include <optional>
#define DEBUG_TYPE "exprconstant"
@@ -6901,51 +6914,113 @@ bool HandleOperatorDeleteCall(EvalInfo &Info, const CallExpr *E) {
//===----------------------------------------------------------------------===//
namespace {
-class BitCastBuffer {
- // FIXME: We're going to need bit-level granularity when we support
- // bit-fields.
+struct BitCastBuffer {
// FIXME: Its possible under the C++ standard for 'char' to not be 8 bits, but
// we don't support a host or target where that is the case. Still, we should
// use a more generic type in case we ever do.
- SmallVector<std::optional<unsigned char>, 32> Bytes;
-
- static_assert(std::numeric_limits<unsigned char>::digits >= 8,
+ using byte_t = unsigned char;
+ static_assert(std::numeric_limits<byte_t>::digits >= 8,
"Need at least 8 bit unsigned char");
+ SmallVector<byte_t, 32> Bytes;
+ SmallVector<byte_t, 32> Valid;
+
bool TargetIsLittleEndian;
-public:
+ static SmallVector<byte_t> MaskAllSet(size_t Width) {
+ SmallVector<byte_t> M;
+ M.resize(Width);
+ std::fill(M.begin(), M.end(), ~0);
+ return M;
+ }
+
BitCastBuffer(CharUnits Width, bool TargetIsLittleEndian)
- : Bytes(Width.getQuantity()),
+ : Bytes(Width.getQuantity()), Valid(Width.getQuantity()),
TargetIsLittleEndian(TargetIsLittleEndian) {}
[[nodiscard]] bool readObject(CharUnits Offset, CharUnits Width,
- SmallVectorImpl<unsigned char> &Output) const {
- for (CharUnits I = Offset, E = Offset + Width; I != E; ++I) {
- // If a byte of an integer is uninitialized, then the whole integer is
- // uninitialized.
- if (!Bytes[I.getQuantity()])
+ SmallVectorImpl<byte_t> &Output,
+ SmallVectorImpl<byte_t> const &Mask) const {
+ assert(Mask.size() >= static_cast<unsigned>(Width.getQuantity()));
+ assert(Output.size() >= static_cast<unsigned>(Width.getQuantity()));
+ assert(Bytes.size() >=
+ static_cast<unsigned>((Offset + Width).getQuantity()));
+
+ SmallVector<byte_t, 8> RevMask;
+ const SmallVectorImpl<byte_t> &M =
+ (llvm::sys::IsLittleEndianHost != TargetIsLittleEndian)
+ ? [&]() -> const SmallVectorImpl<byte_t> & {
+ auto W = Width.getQuantity();
+ RevMask.resize_for_overwrite(W);
+ std::reverse_copy(Mask.begin(), Mask.begin() + W, RevMask.begin());
+ return RevMask;
+ }()
+ : Mask;
+
+ size_t Index = 0;
+ for (CharUnits I = Offset, E = Offset + Width; I != E; ++I, ++Index) {
+ const auto BufIdx = I.getQuantity();
+ const auto mask = M[Index];
+ // are there any bits in Mask[Index] that are not set in
+ // Valid[BufIdx]? (NB: more bits can be set, that's just
+ // fine)
+ if ((Valid[BufIdx] & M[Index]) != M[Index])
+ // If any bit of an integer is uninitialized, then the
+ // whole integer is uninitialized.
return false;
- Output.push_back(*Bytes[I.getQuantity()]);
+
+ Output[Index] = (Output[Index] & ~mask) | (Bytes[BufIdx] & mask);
}
+
if (llvm::sys::IsLittleEndianHost != TargetIsLittleEndian)
std::reverse(Output.begin(), Output.end());
return true;
}
- void writeObject(CharUnits Offset, SmallVectorImpl<unsigned char> &Input) {
- if (llvm::sys::IsLittleEndianHost != TargetIsLittleEndian)
+ void writeObject(CharUnits Offset, SmallVectorImpl<byte_t> &Input,
+ SmallVectorImpl<byte_t> &Mask) {
+ assert(Mask.size() >= Input.size());
+ assert(Bytes.size() >=
+ static_cast<unsigned>(Offset.getQuantity()) + Input.size());
+
+ // we could promise Input and Mask were `const`, except for this
+ if (llvm::sys::IsLittleEndianHost != TargetIsLittleEndian) {
std::reverse(Input.begin(), Input.end());
+ // we might (will) have more mask bits than input bits
+ std::reverse(Mask.begin(), Mask.begin() + Input.size());
+ }
size_t Index = 0;
- for (unsigned char Byte : Input) {
- assert(!Bytes[Offset.getQuantity() + Index] && "overwriting a byte?");
- Bytes[Offset.getQuantity() + Index] = Byte;
+ size_t BufIdx = Offset.getQuantity();
+ for (byte_t &Byte : Input) {
+ assert((Valid[BufIdx] & Mask[Index]) == 0 && "overwriting data?");
+ Bytes[BufIdx] |= Byte & Mask[Index];
+ Valid[BufIdx] |= Mask[Index];
+ ++BufIdx;
++Index;
}
}
size_t size() { return Bytes.size(); }
+
+ LLVM_DUMP_METHOD void dump() {
+ auto pp = [](std::stringstream &SS, llvm::SmallVectorImpl<byte_t> &V) {
+ bool first = true;
+ for (byte_t v : V) {
+ if (first)
+ first = false;
+ else
+ SS << " ";
+ SS << "0x" << std::hex << std::setw(2) << std::setfill('0')
+ << static_cast<unsigned>(v);
+ }
+ };
+ std::stringstream SS[2];
+ pp(SS[0], Bytes);
+ pp(SS[1], Valid);
+ llvm::dbgs() << "BitCastBuffer{Bytes: [" << SS[0].str() << "], Valid: ["
+ << SS[1].str() << "]}\n";
+ }
};
/// Traverse an APValue to produce an BitCastBuffer, emulating how the current
@@ -6973,7 +7048,7 @@ class APValueToBufferConverter {
if (Ty->isNullPtrType())
return true;
- // Dig through Src to find the byte at SrcOffset.
+ // Dig through Val to find the byte at Offset.
switch (Val.getKind()) {
case APValue::Indeterminate:
case APValue::None:
@@ -7012,6 +7087,9 @@ class APValueToBufferConverter {
bool visitRecord(const APValue &Val, QualType Ty, CharUnits Offset) {
const RecordDecl *RD = Ty->getAsRecordDecl();
+ if (RD->isInvalidDecl()) {
+ return invalidDecl(Ty);
+ }
const ASTRecordLayout &Layout = Info.Ctx.getASTRecordLayout(RD);
// Visit the base classes.
@@ -7028,12 +7106,11 @@ class APValueToBufferConverter {
// Visit the fields.
unsigned FieldIdx = 0;
- for (FieldDecl *FD : RD->fields()) {
- if (FD->isBitField()) {
- Info.FFDiag(BCE->getBeginLoc(),
- diag::note_constexpr_bit_cast_unsupported_bitfield);
- return false;
- }
+ for (auto I = RD->field_begin(), E = RD->field_end(); I != E;
+ I++, FieldIdx++) {
+ FieldDecl *FD = *I;
+ if (FD->isBitField())
+ continue; // see below
uint64_t FieldOffsetBits = Layout.getFieldOffset(FieldIdx);
@@ -7044,7 +7121,72 @@ class APValueToBufferConverter {
QualType FieldTy = FD->getType();
if (!visit(Val.getStructField(FieldIdx), FieldTy, FieldOffset))
return false;
- ++FieldIdx;
+ }
+
+ // Handle bit-fields
+ FieldIdx = 0;
+ for (auto I = RD->field_begin(), E = RD->field_end(); I != E;
+ I++, FieldIdx++) {
+ FieldDecl *FD = *I;
+ if (!FD->isBitField())
+ continue;
+
+ // unnamed bit fields are purely padding
+ if (FD->isUnnamedBitfield())
+ continue;
+
+ auto FieldVal = Val.getStructField(FieldIdx);
+ if (!FieldVal.hasValue())
+ continue;
+
+ uint64_t FieldOffsetBits = Layout.getFieldOffset(FieldIdx);
+ CharUnits BufOffset = Offset;
+ uint64_t BitOffset = FieldOffsetBits;
+
+ unsigned int BitWidth = FD->getBitWidthValue(Info.Ctx);
+
+ CharUnits TypeWidth = Info.Ctx.getTypeSizeInChars(FD->getType());
+ uint64_t TypeWidthBits = Info.Ctx.toBits(TypeWidth);
+ if (BitWidth > TypeWidthBits) {
+ // e.g. `unsigned uint8_t c : 12`
+ // we truncate to CHAR_BIT * sizeof(T)
+ // (the extra bits are padding)
+ BitWidth = TypeWidthBits;
+ }
+ if (FieldOffsetBits >= TypeWidthBits) {
+ // e.g. `uint32_t : 33; uint32_t i : 12`
+ // or `uint16_t : 16; unsigned uint16_t i : 12`
+ BufOffset =
+ BufOffset + CharUnits::fromQuantity(BitOffset / TypeWidthBits) *
+ TypeWidth.getQuantity();
+ BitOffset %= TypeWidthBits;
+ }
+
+ if (Info.Ctx.getTargetInfo().isBigEndian()) {
+ // big endian bits count from MSB to LSB
+ // so a bit-field of width 16 and size 12 will occupy bits [0-11] on a
+ // little endian machine, but [3-15] on a big endian machine
+ BitOffset = TypeWidthBits - (BitOffset + BitWidth);
+ }
+
+ assert(TypeWidth >= Info.Ctx.toCharUnitsFromBits(BitWidth));
+
+ llvm::SmallBitVector MaskBits(Info.Ctx.toBits(TypeWidth));
+ MaskBits.set(BitOffset, BitOffset + BitWidth);
+ uintptr_t Store;
+ ArrayRef<uintptr_t> Ref = MaskBits.getData(Store);
+ SmallVector<uint8_t, 8> Mask(Ref.size() * sizeof(uintptr_t));
+ std::memcpy(Mask.data(), Ref.data(), Mask.size());
+ Mask.truncate(TypeWidth.getQuantity());
+
+ SmallVector<uint8_t, 8> Bytes(TypeWidth.getQuantity());
+
+ APSInt Val = FieldVal.getInt() << BitOffset;
+ assert(Val.getBitWidth() >= BitOffset + BitWidth &&
+ "lost data in APInt -> byte buffer conversion");
+
+ llvm::StoreIntToMemory(Val, &*Bytes.begin(), TypeWidth.getQuantity());
+ Buffer.writeObject(BufOffset, Bytes, Mask);
}
return true;
@@ -7129,8 +7271,9 @@ class APValueToBufferConverter {
}
SmallVector<uint8_t, 8> Bytes(NElts / 8);
+ auto Mask = BitCastBuffer::MaskAllSet(Bytes.size());
llvm::StoreIntToMemory(Res, &*Bytes.begin(), NElts / 8);
- Buffer.writeObject(Offset, Bytes);
+ Buffer.writeObject(Offset, Bytes, Mask);
} else {
// Iterate over each of the elements and write them out to the buffer at
// the appropriate offset.
@@ -7153,8 +7296,9 @@ class APValueToBufferConverter {
}
SmallVector<uint8_t, 8> Bytes(Width / 8);
+ auto Mask = BitCastBuffer::MaskAllSet(Bytes.size());
llvm::StoreIntToMemory(AdjustedVal, &*Bytes.begin(), Width / 8);
- Buffer.writeObject(Offset, Bytes);
+ Buffer.writeObject(Offset, Bytes, Mask);
return true;
}
@@ -7163,6 +7307,12 @@ class APValueToBufferConverter {
return visitInt(AsInt, Ty, Offset);
}
+ bool invalidDecl(QualType Ty) {
+ Info.FFDiag(BCE->getBeginLoc(), diag::note_constexpr_bit_cast_invalid_decl)
+ << /* checking dest */ false << Ty;
+ return false;
+ }
+
public:
static std::optional<BitCastBuffer>
convert(EvalInfo &Info, const APValue &Src, const CastExpr *BCE) {
@@ -7194,6 +7344,12 @@ class BufferToAPValueConverter {
return std::nullopt;
}
+ std::nullopt_t invalidDecl(QualType Ty) {
+ Info.FFDiag(BCE->getBeginLoc(), diag::note_constexpr_bit_cast_invalid_decl)
+ << /* checking dest */ true << Ty;
+ return std::nullopt;
+ }
+
std::nullopt_t unrepresentableValue(QualType Ty, const APSInt &Val) {
Info.FFDiag(BCE->getBeginLoc(),
diag::note_constexpr_bit_cast_unrepresentable_value)
@@ -7201,6 +7357,75 @@ class BufferToAPValueConverter {
return std::nullopt;
}
+ std::nullopt_t badBits(QualType Ty, CharUnits Offset,
+ SmallVectorImpl<BitCastBuffer::byte_t> &M) {
+ Info.FFDiag(BCE->getExprLoc(), diag::note_constexpr_bit_cast_indet_dest, 1)
+ << Ty << Info.Ctx.getLangOpts().CharIsSigned;
+ uint64_t BitWidth = Info.Ctx.getTypeSize(BCE->getType());
+ uint64_t ByteWidth = Info.Ctx.toCharUnitsFromBits(BitWidth).getQuantity();
+ assert(ByteWidth == Buffer.Valid.size_in_bytes());
+
+ APInt Valid(BitWidth, 0);
+ llvm::LoadIntFromMemory(Valid, Buffer.Valid.begin(), ByteWidth);
+ APInt Mask(BitWidth, 0);
+ llvm::LoadIntFromMemory(Mask, M.begin(), M.size_in_bytes());
+
+ Mask = Mask.zext(Valid.getBitWidth());
+ Mask <<= Info.Ctx.toBits(Offset);
+
+ auto ByteAligned = true;
+
+ APInt Missing = (~Valid & Mask);
+ assert(!Missing.isZero() && "bad bits called with no bad bits?");
+ llvm::SmallVector<std::pair<size_t, size_t>> MissingBitRanges;
+ int NextBit = 0;
+ while (!Missing.isZero()) {
+ APInt Last(Missing);
+ int N = Missing.countr_zero();
+
+ Missing.lshrInPlace(N);
+ auto M = Missing.countr_one();
+
+ MissingBitRanges.push_back({NextBit + N, NextBit + N + M});
+
+ Missing.lshrInPlace(M);
+ NextBit += N;
+ NextBit += M;
+ ByteAligned &= N % Info.Ctx.getCharWidth() == 0;
+ ByteAligned &= M % Info.Ctx.getCharWidth() == 0;
+ }
+
+ llvm::SmallString<32> RangesStr;
+ llvm::raw_svector_ostream OS(RangesStr);
+ bool First = true;
+ for (auto [Start, End] : MissingBitRanges) {
+ if (!First)
+ OS << " ";
+ else
+ First = false;
+ if (ByteAligned) {
+ Start /= Info.Ctx.getCharWidth();
+ End /= Info.Ctx.getCharWidth();
+ }
+ size_t Len = End - Start;
+ if (Len > 1) {
+ OS << Start << "-" << End - 1;
+ } else {
+ OS << Start;
+ }
+ }
+
+ assert(RangesStr.size() > 0);
+ auto LastIdx = (ByteAligned ? ByteWidth : BitWidth) - 1;
+ bool IsForSubobject =
+ BCE->getType().getCanonicalType() != Ty.getCanonicalType();
+ Info.Note(BCE->getSubExpr()->getExprLoc(),
+ diag::note_constexpr_bit_cast_bad_bits)
+ << ByteAligned << RangesStr << LastIdx << Ty << BCE->getType()
+ << BCE->getSubExpr()->getType() << IsForSubobject;
+ return std::nullopt;
+ }
+
std::optional<APValue> visit(const BuiltinType *T, CharUnits Offset,
const EnumType *EnumSugar = nullptr) {
if (T->isNullPtrType()) {
@@ -7225,8 +7450,10 @@ class BufferToAPValueConverter {
SizeOf = NumBytes;
}
- SmallVector<uint8_t, 8> Bytes;
- if (!Buffer.readObject(Offset, SizeOf, Bytes)) {
+ SmallVector<uint8_t, 8> Bytes,
+ Mask = BitCastBuffer::MaskAllSet(SizeOf.getQuantity());
+ Bytes.resize_for_overwrite(SizeOf.getQuantity());
+ if (!Buffer.readObject(Offset, SizeOf, Bytes, Mask)) {
// If this is std::byte or unsigned char, then its okay to store an
// indeterminate value.
bool IsStdByte = EnumSugar && EnumSugar->isStdByteType();
@@ -7235,10 +7462,7 @@ class BufferToAPValueConverter {
T->isSpecificBuiltinType(BuiltinType::Char_U));
if (!IsStdByte && !IsUChar) {
QualType DisplayType(EnumSugar ? (const Type *)EnumSugar : T, 0);
- Info.FFDiag(BCE->getExprLoc(),
- diag::note_constexpr_bit_cast_indet_dest)
- << DisplayType << Info.Ctx.getLangOpts().CharIsSigned;
- return std::nullopt;
+ return badBits(DisplayType, Offset, Mask);
}
return APValue::IndeterminateValue();
@@ -7272,6 +7496,9 @@ class BufferToAPValueConverter {
std::optional<APValue> visit(const RecordType *RTy, CharUnits Offset) {
const RecordDecl *RD = RTy->getAsRecordDecl();
+ if (RD->isInvalidDecl()) {
+ return invalidDecl(QualType(RD->getTypeForDecl(), 0));
+ }
const ASTRecordLayout &Layout = Info.Ctx.getASTRecordLayout(RD);
unsigned NumBases = 0;
@@ -7300,14 +7527,11 @@ class BufferToAPValueConverter {
// Visit the fields.
unsigned FieldIdx = 0;
- for (FieldDecl *FD : RD->fields()) {
- // FIXME: We don't currently support bit-fields. A lot of the logic for
- // this is in CodeGen, so we need to factor it around.
- if (FD->isBitField()) {
- Info.FFDiag(BCE->getBeginLoc(),
- diag::note_constexpr_bit_cast_unsupported_bitfield);
- return std::nullopt;
- }
+ for (auto I = RD->field_begin(), E = RD->field_end(); I != E;
+ I++, FieldIdx++) {
+ FieldDecl *FD = *I;
+ if (FD->isBitField())
+ continue; // see below
uint64_t FieldOffsetBits = Layout.getFieldOffset(FieldIdx);
assert(FieldOffsetBits % Info.Ctx.getCharWidth() == 0);
@@ -7320,7 +7544,86 @@ class BufferToAPValueConverter {
if (!SubObj)
return std::nullopt;
ResultVal.getStructField(FieldIdx) = *SubObj;
- ++FieldIdx;
+ }
+
+ // Handle bit-fields
+ FieldIdx = 0;
+ for (auto I = RD->field_begin(), E = RD->field_end(); I != E;
+ I++, FieldIdx++) {
+ FieldDecl *FD = *I;
+ if (!FD->isBitField())
+ continue;
+
+ // unnamed bit fields are purely padding
+ if (FD->isUnnamedBitfield())
+ continue;
+
+ uint64_t FieldOffsetBits = Layout.getFieldOffset(FieldIdx);
+ CharUnits BufOffset = Offset;
+ uint64_t BitOffset = FieldOffsetBits;
+
+ unsigned int BitWidth = FD->getBitWidthValue(Info.Ctx);
+
+ CharUnits TypeWidth = Info.Ctx.getTypeSizeInChars(FD->getType());
+ uint64_t TypeWidthBits = Info.Ctx.toBits(TypeWidth);
+ if (BitWidth > TypeWidthBits) {
+ // e.g. `unsigned uint8_t c : 12`
+ // we truncate to CHAR_BIT * sizeof(T)
+ // (the extra bits are padding)
+ BitWidth = TypeWidthBits;
+ }
+ if (FieldOffsetBits >= TypeWidthBits) {
+ // e.g. `uint32_t : 33; uint32_t i : 12`
+ // or `uint16_t : 16; unsigned uint16_t i : 12`
+ BufOffset =
+ BufOffset + CharUnits::fromQuantity(BitOffset / TypeWidthBits) *
+ TypeWidth.getQuantity();
+ BitOffset %= TypeWidthBits;
+ }
+
+ if (Info.Ctx.getTargetInfo().isBigEndian()) {
+ // big endian bits count from MSB to LSB
+ // so a bit-field of width 16 and size 12 will occupy bits [0-11] on a
+ // little endian machine, but [3-15] on a big endian machine
+ BitOffset = TypeWidthBits - (BitOffset + BitWidth);
+ }
+
+ assert(TypeWidth >= Info.Ctx.toCharUnitsFromBits(BitWidth));
+
+ llvm::Small...
[truncated]
|
sethp
commented
Dec 8, 2023
|
I don't have the capacity to review this properly, but the changes to the |
|
Easy enough: I opened a PR for just those changes as #74852 . For now, I hope it's OK if I leave them here too as I continue working on the bit-cast part: I'd still like to have them locally, and there's a lot of |
sethp
changed the title
|
Happy new year! An update from my end: I've made what feels like good progress on refactoring the implementation to be a little more concise, both in terms of code and dynamic instruction count (especially for large types). There's still a little more I'd like to do there, especially as relates to handling host-target endianness mismatches: and I think at least one more test I need to devise to validate that the implementation handles bit-fields split across a byte boundary correctly. I hope to push that work up soon, but it still feels a little "too early" to be properly respectful of y'all's time. However, in doing that work, I identified an issue with |
Make use of the pre-exising APInt type as a pre-established spelling of BitVector to get much more granular with the BitCastBuffer; this also permits more thorough usage of `const APInt&` references to make the copies easier to optimize away and reduces the common case of int->buffer->int from 4-7 copies down to 2. As a bonus, we also got a bit more flexible with supporting platforms where `CHAR_BIT` is something other than 8, addressing some of the review feedback from the original diff, though there's plenty of follow-up work here and elsewhere to make that a reality. Additional, uncovers and fixes some errant behavior around `bool`s introduced in the previous commit, and makes the handling of `_BitInt(N)` types more consistent while preserving backwards-compatability.
sethp
commented
Jan 5, 2024
Comment on lines
+697
to
+698
| static_assert(round_trip<signed char>(true)); | ||
| static_assert(round_trip<unsigned char>(true)); |
There was a problem hiding this comment.
These tests document the existing behavior, but both this and the bit_cast<uint8_t>(false) below are somewhat troublesome in that we're promoting the bool's padding bits to "real" bits (by zero-extension). As I understand it, that's allowed by the spec (padding bits are "unspecified"), but it does mean that we've got to "over-read" bools as being a full i8 when we convert bit_cast<bool>((uint8_t)N) so that we can refuse to produce a constant value if the top bits are non-zero.
It would be more consistent and simplify the implementation a bit to throw an error on bit_cast<char>(false) in a constexpr context complaining about attempting to read padding bits, but I've chosen to preserve the existing behavior here in service of backwards compatibility and usability.
sethp
commented
Jan 5, 2024
Comment on lines
+30
to
+33
| static_assert(bit_cast<uint8_t, _BitInt(8)>(0xff) == 0xff); | ||
| template <int N> struct bytes { uint8_t b[N]; }; | ||
| static_assert(bit_cast<bytes<2>, _BitInt(12)>(0xff).b[(LITTLE_END ? 0 : /* fixme */ 0)] == 0xff); | ||
| static_assert(bit_cast<bytes<4>, _BitInt(24)>(0xff).b[(LITTLE_END ? 0 : /* fixme */ 2)] == 0xff); |
There was a problem hiding this comment.
These tests also document the existing (buggy) behavior; I don't expect there's a lot of people out there using constexpr std::bit_cast on _BitInt(N) types, but if there are they're having a bad time with it.
That said, I took pains to preserve the existing behavior to try and make this change purely "additive"—to the best of my ability I tried to preserve the property that no code that previously compiled without error (including badly formed constructs such as these) would start producing an error.
That's a pretty strong stance though, that I went with because I couldn't find details on clang's backwards compatibility policy. I'm happy to soften it: in this case, I would probably prefer to produce an error instead of a value from APValueToBufferConverter when it encounders a _BitInt(N) type and thereby make it consistently unsupported pending future work.
sethp
commented
Jan 5, 2024
Comment on lines
+782
to
+794
| // `bool` includes padding bits, but *which* single bit stores the | ||
| // value is under-specified. These tests not-so-secretly assert that | ||
| // it's in fact the LSB that the compiler "sees" as the value. | ||
| struct pack { | ||
| bool a : 1; | ||
| bool b : 1; | ||
|
|
||
| // 1 bit of value, 5 bits of padding | ||
| bool c : 6; | ||
| }; | ||
|
|
||
| constexpr auto packed = bit_cast<pack, uint8_t>(LITTLE_END ? 0x07 : 0xc1); | ||
| static_assert(packed.a && packed.b && packed.c); |
There was a problem hiding this comment.
Taken together this makes a very strong assertion about the ABI of a boolean; that seems to match the existing behavior, both in how the evaulator handled 8-bit values and the output from codegen: https://godbolt.org/z/xsjE7aG3e
Probably there's no good reason to make a different decision in the future, but it does seem worth calling out: this is far from the only place the evaluator rests on the decisions made elsewhere in clang/llvm-land, but it's (partially) a new one.
sethp
commented
Jan 5, 2024
Comment on lines
883
to
884
| // expected-error@+2 {{constexpr variable 'bad_bool9_to_short' must be initialized by a constant expression}} | ||
| // expected-note@+1 {{bit_cast involving type 'bool __attribute__((ext_vector_type(9)))' (vector of 9 'bool' values) is not allowed in a constant expression; element size 1 * element count 9 is not a multiple of the byte size 8}} |
There was a problem hiding this comment.
I don't know if it's important or not, but this would be pretty easy to support with the new bit-wise APInt-backed BitCastBuffer: I didn't try for it because there's already quite a lot going on here, but I'm happy to follow up on that if it seems worthwhile.
sethp
commented
Jan 5, 2024
Comment on lines
+6942
to
+6943
| static BitFieldInfo MakeInfo(const ASTContext &Ctx, const FieldDecl *FD, | ||
| const ASTRecordLayout &Layout) { |
There was a problem hiding this comment.
This is part-way to the aforementioned refactoring of the bit-field layout & access logic between AST and codegen; this MakeInfo method happens to make the same choices that codegen does, but those choices seem very consistent across time and even compilers (sorry, I lost the godbolt link for this one).
Really it's only two:
- What happens with "oversized" bit-fields, and
- What "direction" does the offset count on little-endian vs. big-endian?
At this point, the answers seem consistent; most of the complicated stuff already got handled in setting up the bit-wise field offsets in the first place.
That said, at this point I'm a little stuck: I can move logic around between here, CGRecordBuilder/CGRecordLowering, and AST/RecordLayoutBuilder.cpp, but I don't have a sense of clarity around the outcome I'd be looking for there. I suspect the goal is shaped something like "someone looking to answer those two questions differently affects the constant evaluator with their changes", but I'm not sure how I'd achieve that.
sethp
commented
Jan 5, 2024
Comment on lines
+7126
to
+7127
| inline static void copyBitsFrom(APInt &LHS, const BitSlice &Dst, | ||
| const APInt &RHS, const BitSlice &Src) { |
There was a problem hiding this comment.
This has roughly the right API shape to it, I feel, but it's a bit of a mismatch between this and the existing APInt insertBits method, especially since the latter contains this snippet:
// General case - set/clear individual bits in dst based on src.
// TODO - there is scope for optimization here, but at the moment this code
// path is barely used so prefer readability over performance.
for (unsigned i = 0; i != subBitWidth; ++i)
setBitVal(bitPosition + i, subBits[i]);
We'd fairly often be falling through to that "general" implementation here, so I went ahead and improved matters some in sethp#3 : but that's an time-domain-effect-only change, and not functionally required, so my plan is to wait for this PR to land before proposing those optimizations. I'm noting it here just in case you also happen to notice that little wrinkle.
tbaederr
requested review from
zygoloid,
cor3ntin,
AaronBallman and
erichkeane
|
This PR should fix #54018. |
tbaederr
added a commit
that referenced
this pull request
Oct 31, 2024
This is a subset of #68288, with hopefully narrower scope. It does not support bitcasting to non-integral types yet. Bitfields are supported, but only if they result in a full byte-sized final buffer. It does not support casting from null-pointers yet or casting from indeterminate bits. The tests are from #68288 and partially from #74775. The `BitcastBuffer` struct is currently always working in single bits, but I plan to (try to) optimize this for the common full-byte case.
smallp-o-p
pushed a commit
to smallp-o-p/llvm-project
that referenced
this pull request
Nov 3, 2024
This is a subset of llvm#68288, with hopefully narrower scope. It does not support bitcasting to non-integral types yet. Bitfields are supported, but only if they result in a full byte-sized final buffer. It does not support casting from null-pointers yet or casting from indeterminate bits. The tests are from llvm#68288 and partially from llvm#74775. The `BitcastBuffer` struct is currently always working in single bits, but I plan to (try to) optimize this for the common full-byte case.
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
This is a subset of llvm#68288, with hopefully narrower scope. It does not support bitcasting to non-integral types yet. Bitfields are supported, but only if they result in a full byte-sized final buffer. It does not support casting from null-pointers yet or casting from indeterminate bits. The tests are from llvm#68288 and partially from llvm#74775. The `BitcastBuffer` struct is currently always working in single bits, but I plan to (try to) optimize this for the common full-byte case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After this commit, clang permits constructions like:
A few notes for the reviewers:
It's "stacked" on top of a change makingSplit out into [Clang][Sema] Print more static_assert exprs #74852static_assertprint more complex expressions and binary operator call operands because that makes it a whole lot easier to debug, and it seems like there's a lot of overlap between folks working in both areas. I tried to open an actual "stacked" pair of PRs per the latest on Write a guide for doing "Stacked Reviews" with GitHub Pull Requests #56636 , but I don't have the prerequisite permission to push tousers/sethp/...and I'm not quite sure how to acquire it; if this doesn't work for you, just let me know what you'd prefer.constexpr-builtin-bit-cast.cpp); I'm especially happy that I worked out how to get the compiler to indicate which bits were indeterminate, because that really helped me learn and understand the way the structures were being laid out in memory (especially when paired with the "stacked" change).I also have an inkling thatI think this part at least worked out well in 2e0d3f8APIntis a better spelling of "fixed-width bit-buffer" thanSmallVector, especially for this situation where we're (so far) exclusively mapping to/fromAPInts.To better inform the shape of work that I see as outstanding here, I was hoping to solicit your feedback on three specific points:
APSInt. That's convenient for a couple reasons (e.g. there's no need for "implicit" up/down-cast nodes in the expression tree), but it does mean that we've got to invent values for some bits that really aren't "part" of the bit-field. The least surprising way I found to do this was via sign-extension, but that does lose track of information: for example, givenstruct { signed : 1 ; signed : 31 } S,std::bit_cast<signed>(S)doesn't work in aconstexprcontext, butstd::bit_cast<signed>(Ex.signed)does. I'm not really sure what there is to be done about this one besides changing the evaluator to be bit-width-precise, which seems like a larger unit of work. It's also not really new to this change, but it's something that I noticed that seemed worth raising.unsigned char,std::byte). I think I can fix the former by computing the entire "mask" in one pass and then applying it in the next, so we can do a total validity check once (instead of once per field). I'm a little stumped about the latter, though, since it seems like it'd mostly just be false positives: is it possible to add some sort of "deferred" note that doesn't get emitted unless they try to use the uninitialized bytes somehow?Thanks for all the hard work y'all do!