[clang][bytecode] Allocate IntegralAP and Floating types using an allocator #144246
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
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Timm Baeder (tbaederr) ChangesBoth This is why clang has In the bytecode interpreter, we have a similar problem. Large integers and floating-point values are placement-new allocated into the Fix this by using an approach similar to the one used in the AST. Add an allocator to Interestingly, this results in a slight improvement in compile times: https://llvm-compile-time-tracker.com/compare.php?from=6bfcdda9b1ddf0900f82f7e30cb5e3253a791d50&to=88d1d899127b408f0fb0f385c2c58e6283195049&stat=instructions:u (but don't ask my why). Patch is 83.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/144246.diff 17 Files Affected:
diff --git a/clang/lib/AST/ByteCode/Compiler.cpp b/clang/lib/AST/ByteCode/Compiler.cpp
index bf38b2e5d537d..136bb46d2a516 100644
--- a/clang/lib/AST/ByteCode/Compiler.cpp
+++ b/clang/lib/AST/ByteCode/Compiler.cpp
@@ -748,7 +748,8 @@ bool Compiler<Emitter>::VisitFloatingLiteral(const FloatingLiteral *E) {
if (DiscardResult)
return true;
- return this->emitConstFloat(E->getValue(), E);
+ APFloat F = E->getValue();
+ return this->emitFloat(F, E);
}
template <class Emitter>
@@ -4185,8 +4186,10 @@ bool Compiler<Emitter>::visitZeroInitializer(PrimType T, QualType QT,
nullptr, E);
case PT_MemberPtr:
return this->emitNullMemberPtr(0, nullptr, E);
- case PT_Float:
- return this->emitConstFloat(APFloat::getZero(Ctx.getFloatSemantics(QT)), E);
+ case PT_Float: {
+ APFloat F = APFloat::getZero(Ctx.getFloatSemantics(QT));
+ return this->emitFloat(F, E);
+ }
case PT_FixedPoint: {
auto Sem = Ctx.getASTContext().getFixedPointSemantics(E->getType());
return this->emitConstFixedPoint(FixedPoint::zero(Sem), E);
@@ -4674,10 +4677,7 @@ VarCreationState Compiler<Emitter>::visitVarDecl(const VarDecl *VD,
if (!visitInitializer(Init))
return false;
- if (!this->emitFinishInit(Init))
- return false;
-
- return this->emitPopPtr(Init);
+ return this->emitFinishInitGlobal(Init);
};
DeclScope<Emitter> LocalScope(this, VD);
@@ -4698,51 +4698,45 @@ VarCreationState Compiler<Emitter>::visitVarDecl(const VarDecl *VD,
return false;
return !Init || (checkDecl() && initGlobal(*GlobalIndex));
- } else {
- InitLinkScope<Emitter> ILS(this, InitLink::Decl(VD));
-
- if (VarT) {
- unsigned Offset = this->allocateLocalPrimitive(
- VD, *VarT, VD->getType().isConstQualified(), nullptr,
- ScopeKind::Block, IsConstexprUnknown);
- if (Init) {
- // If this is a toplevel declaration, create a scope for the
- // initializer.
- if (Toplevel) {
- LocalScope<Emitter> Scope(this);
- if (!this->visit(Init))
- return false;
- return this->emitSetLocal(*VarT, Offset, VD) && Scope.destroyLocals();
- } else {
- if (!this->visit(Init))
- return false;
- return this->emitSetLocal(*VarT, Offset, VD);
- }
- }
- } else {
- if (std::optional<unsigned> Offset =
- this->allocateLocal(VD, VD->getType(), nullptr, ScopeKind::Block,
- IsConstexprUnknown)) {
- if (!Init)
- return true;
+ }
+ // Local variables.
+ InitLinkScope<Emitter> ILS(this, InitLink::Decl(VD));
- if (!this->emitGetPtrLocal(*Offset, Init))
+ if (VarT) {
+ unsigned Offset = this->allocateLocalPrimitive(
+ VD, *VarT, VD->getType().isConstQualified(), nullptr, ScopeKind::Block,
+ IsConstexprUnknown);
+ if (Init) {
+ // If this is a toplevel declaration, create a scope for the
+ // initializer.
+ if (Toplevel) {
+ LocalScope<Emitter> Scope(this);
+ if (!this->visit(Init))
return false;
-
- if (!visitInitializer(Init))
+ return this->emitSetLocal(*VarT, Offset, VD) && Scope.destroyLocals();
+ } else {
+ if (!this->visit(Init))
return false;
+ return this->emitSetLocal(*VarT, Offset, VD);
+ }
+ }
+ } else {
+ if (std::optional<unsigned> Offset = this->allocateLocal(
+ VD, VD->getType(), nullptr, ScopeKind::Block, IsConstexprUnknown)) {
+ if (!Init)
+ return true;
- if (!this->emitFinishInit(Init))
- return false;
+ if (!this->emitGetPtrLocal(*Offset, Init))
+ return false;
- return this->emitPopPtr(Init);
- }
- return false;
+ if (!visitInitializer(Init))
+ return false;
+
+ return this->emitFinishInitPop(Init);
}
- return true;
+ return false;
}
-
- return false;
+ return true;
}
template <class Emitter>
@@ -4751,8 +4745,10 @@ bool Compiler<Emitter>::visitAPValue(const APValue &Val, PrimType ValType,
assert(!DiscardResult);
if (Val.isInt())
return this->emitConst(Val.getInt(), ValType, E);
- else if (Val.isFloat())
- return this->emitConstFloat(Val.getFloat(), E);
+ else if (Val.isFloat()) {
+ APFloat F = Val.getFloat();
+ return this->emitFloat(F, E);
+ }
if (Val.isLValue()) {
if (Val.isNullPointer())
@@ -6133,8 +6129,10 @@ bool Compiler<Emitter>::VisitUnaryOperator(const UnaryOperator *E) {
const auto &TargetSemantics = Ctx.getFloatSemantics(E->getType());
if (!this->emitLoadFloat(E))
return false;
- if (!this->emitConstFloat(llvm::APFloat(TargetSemantics, 1), E))
+ APFloat F(TargetSemantics, 1);
+ if (!this->emitFloat(F, E))
return false;
+
if (!this->emitAddf(getFPOptions(E), E))
return false;
if (!this->emitStoreFloat(E))
@@ -6176,8 +6174,10 @@ bool Compiler<Emitter>::VisitUnaryOperator(const UnaryOperator *E) {
const auto &TargetSemantics = Ctx.getFloatSemantics(E->getType());
if (!this->emitLoadFloat(E))
return false;
- if (!this->emitConstFloat(llvm::APFloat(TargetSemantics, 1), E))
+ APFloat F(TargetSemantics, 1);
+ if (!this->emitFloat(F, E))
return false;
+
if (!this->emitSubf(getFPOptions(E), E))
return false;
if (!this->emitStoreFloat(E))
@@ -6957,6 +6957,20 @@ bool Compiler<Emitter>::emitDummyPtr(const DeclTy &D, const Expr *E) {
return true;
}
+template <class Emitter>
+bool Compiler<Emitter>::emitFloat(const APFloat &F, const Expr *E) {
+ assert(!DiscardResult && "Should've been checked before");
+
+ if (Floating::singleWord(F.getSemantics()))
+ return this->emitConstFloat(Floating(F), E);
+
+ APInt I = F.bitcastToAPInt();
+ return this->emitConstFloat(
+ Floating(const_cast<uint64_t *>(I.getRawData()),
+ llvm::APFloatBase::SemanticsToEnum(F.getSemantics())),
+ E);
+}
+
// This function is constexpr if and only if To, From, and the types of
// all subobjects of To and From are types T such that...
// (3.1) - is_union_v<T> is false;
diff --git a/clang/lib/AST/ByteCode/Compiler.h b/clang/lib/AST/ByteCode/Compiler.h
index ac3ad84766dc6..a1d068cc7e0ae 100644
--- a/clang/lib/AST/ByteCode/Compiler.h
+++ b/clang/lib/AST/ByteCode/Compiler.h
@@ -391,6 +391,7 @@ class Compiler : public ConstStmtVisitor<Compiler<Emitter>, bool>,
bool emitRecordDestruction(const Record *R, SourceInfo Loc);
bool emitDestruction(const Descriptor *Desc, SourceInfo Loc);
bool emitDummyPtr(const DeclTy &D, const Expr *E);
+ bool emitFloat(const APFloat &F, const Expr *E);
unsigned collectBaseOffset(const QualType BaseType,
const QualType DerivedType);
bool emitLambdaStaticInvokerBody(const CXXMethodDecl *MD);
diff --git a/clang/lib/AST/ByteCode/Descriptor.cpp b/clang/lib/AST/ByteCode/Descriptor.cpp
index 5531295dfa2f8..46e4d0d940b3e 100644
--- a/clang/lib/AST/ByteCode/Descriptor.cpp
+++ b/clang/lib/AST/ByteCode/Descriptor.cpp
@@ -368,7 +368,7 @@ Descriptor::Descriptor(const DeclTy &D, PrimType Type, MetadataSize MD,
bool IsTemporary, bool IsConst, UnknownSize)
: Source(D), ElemSize(primSize(Type)), Size(UnknownSizeMark),
MDSize(MD.value_or(0)),
- AllocSize(MDSize + sizeof(InitMapPtr) + alignof(void *)),
+ AllocSize(MDSize + sizeof(InitMapPtr) + alignof(void *)), PrimT(Type),
IsConst(IsConst), IsMutable(false), IsTemporary(IsTemporary),
IsArray(true), CtorFn(getCtorArrayPrim(Type)),
DtorFn(getDtorArrayPrim(Type)), MoveFn(getMoveArrayPrim(Type)) {
diff --git a/clang/lib/AST/ByteCode/Disasm.cpp b/clang/lib/AST/ByteCode/Disasm.cpp
index 846dc2fe92a70..7c6b78386b14f 100644
--- a/clang/lib/AST/ByteCode/Disasm.cpp
+++ b/clang/lib/AST/ByteCode/Disasm.cpp
@@ -50,34 +50,56 @@ inline static std::string printArg(Program &P, CodePtr &OpPC) {
}
template <> inline std::string printArg<Floating>(Program &P, CodePtr &OpPC) {
- auto F = Floating::deserialize(*OpPC);
- OpPC += align(F.bytesToSerialize());
+ auto Sem = Floating::deserializeSemantics(*OpPC);
- std::string Result;
- llvm::raw_string_ostream SS(Result);
- SS << F;
- return Result;
+ unsigned BitWidth = llvm::APFloatBase::semanticsSizeInBits(
+ llvm::APFloatBase::EnumToSemantics(Sem));
+ auto Memory =
+ std::make_unique<uint64_t[]>(llvm::APInt::getNumWords(BitWidth));
+ Floating Result(Memory.get(), Sem);
+ Floating::deserialize(*OpPC, &Result);
+
+ OpPC += align(Result.bytesToSerialize());
+
+ std::string S;
+ llvm::raw_string_ostream SS(S);
+ SS << Result;
+ return S;
}
template <>
inline std::string printArg<IntegralAP<false>>(Program &P, CodePtr &OpPC) {
- auto F = IntegralAP<false>::deserialize(*OpPC);
- OpPC += align(F.bytesToSerialize());
-
- std::string Result;
- llvm::raw_string_ostream SS(Result);
- SS << F;
- return Result;
+ using T = IntegralAP<false>;
+ unsigned BitWidth = T::deserializeSize(*OpPC);
+ auto Memory =
+ std::make_unique<uint64_t[]>(llvm::APInt::getNumWords(BitWidth));
+
+ T Result(Memory.get(), BitWidth);
+ T::deserialize(*OpPC, &Result);
+
+ OpPC += Result.bytesToSerialize();
+ std::string Str;
+ llvm::raw_string_ostream SS(Str);
+ SS << Result;
+ return Str;
}
+
template <>
inline std::string printArg<IntegralAP<true>>(Program &P, CodePtr &OpPC) {
- auto F = IntegralAP<true>::deserialize(*OpPC);
- OpPC += align(F.bytesToSerialize());
+ using T = IntegralAP<true>;
+ unsigned BitWidth = T::deserializeSize(*OpPC);
+ auto Memory =
+ std::make_unique<uint64_t[]>(llvm::APInt::getNumWords(BitWidth));
- std::string Result;
- llvm::raw_string_ostream SS(Result);
- SS << F;
- return Result;
+ T Result(Memory.get(), BitWidth);
+ T::deserialize(*OpPC, &Result);
+
+ std::string Str;
+ llvm::raw_string_ostream SS(Str);
+ SS << Result;
+
+ OpPC += Result.bytesToSerialize();
+ return Str;
}
template <> inline std::string printArg<FixedPoint>(Program &P, CodePtr &OpPC) {
diff --git a/clang/lib/AST/ByteCode/Floating.h b/clang/lib/AST/ByteCode/Floating.h
index 3750568fc23c7..f67c898f48eb1 100644
--- a/clang/lib/AST/ByteCode/Floating.h
+++ b/clang/lib/AST/ByteCode/Floating.h
@@ -17,63 +17,80 @@
#include "clang/AST/APValue.h"
#include "llvm/ADT/APFloat.h"
+// XXX This is just a debugging help. Setting this to 1 will heap-allocate ALL
+// floating values.
+#define ALLOCATE_ALL 0
+
namespace clang {
namespace interp {
using APFloat = llvm::APFloat;
using APSInt = llvm::APSInt;
+using APInt = llvm::APInt;
+/// If a Floating is constructed from Memory, it DOES NOT OWN THAT MEMORY.
+/// It will NOT copy the memory (unless, of course, copy() is called) an it
+/// won't alllocate anything. The allocation should happen via InterpState or
+/// Program.
class Floating final {
private:
- // The underlying value storage.
- APFloat F;
+ union {
+ uint64_t Val = 0;
+ uint64_t *Memory;
+ };
+ llvm::APFloatBase::Semantics Semantics =
+ llvm::APFloatBase::Semantics::S_IEEEhalf;
+
+ APFloat getValue() const {
+ unsigned BitWidth = bitWidth();
+ if (singleWord())
+ return APFloat(getSemantics(), APInt(BitWidth, Val));
+ unsigned NumWords = numWords();
+ return APFloat(getSemantics(), APInt(BitWidth, NumWords, Memory));
+ }
public:
- /// Zero-initializes a Floating.
- Floating() : F(0.0f) {}
- Floating(const APFloat &F) : F(F) {}
+ Floating() = default;
+ Floating(llvm::APFloatBase::Semantics Semantics)
+ : Val(0), Semantics(Semantics) {}
+ Floating(const APFloat &F) {
- // Static constructors for special floating point values.
- static Floating getInf(const llvm::fltSemantics &Sem) {
- return Floating(APFloat::getInf(Sem));
+ Semantics = llvm::APFloatBase::SemanticsToEnum(F.getSemantics());
+ this->copy(F);
}
- const APFloat &getAPFloat() const { return F; }
+ Floating(uint64_t *Memory, llvm::APFloatBase::Semantics Semantics)
+ : Memory(Memory), Semantics(Semantics) {}
+
+ APFloat getAPFloat() const { return getValue(); }
- bool operator<(Floating RHS) const { return F < RHS.F; }
- bool operator>(Floating RHS) const { return F > RHS.F; }
- bool operator<=(Floating RHS) const { return F <= RHS.F; }
- bool operator>=(Floating RHS) const { return F >= RHS.F; }
- bool operator==(Floating RHS) const { return F == RHS.F; }
- bool operator!=(Floating RHS) const { return F != RHS.F; }
- Floating operator-() const { return Floating(-F); }
+ bool operator<(Floating RHS) const { return getValue() < RHS.getValue(); }
+ bool operator>(Floating RHS) const { return getValue() > RHS.getValue(); }
+ bool operator<=(Floating RHS) const { return getValue() <= RHS.getValue(); }
+ bool operator>=(Floating RHS) const { return getValue() >= RHS.getValue(); }
APFloat::opStatus convertToInteger(APSInt &Result) const {
bool IsExact;
- return F.convertToInteger(Result, llvm::APFloat::rmTowardZero, &IsExact);
+ return getValue().convertToInteger(Result, llvm::APFloat::rmTowardZero,
+ &IsExact);
}
- Floating toSemantics(const llvm::fltSemantics *Sem,
- llvm::RoundingMode RM) const {
- APFloat Copy = F;
+ void toSemantics(const llvm::fltSemantics *Sem, llvm::RoundingMode RM,
+ Floating *Result) const {
+ APFloat Copy = getValue();
bool LosesInfo;
Copy.convert(*Sem, RM, &LosesInfo);
(void)LosesInfo;
- return Floating(Copy);
- }
-
- /// Convert this Floating to one with the same semantics as \Other.
- Floating toSemantics(const Floating &Other, llvm::RoundingMode RM) const {
- return toSemantics(&Other.F.getSemantics(), RM);
+ Result->copy(Copy);
}
APSInt toAPSInt(unsigned NumBits = 0) const {
- return APSInt(F.bitcastToAPInt());
+ return APSInt(getValue().bitcastToAPInt());
}
- APValue toAPValue(const ASTContext &) const { return APValue(F); }
+ APValue toAPValue(const ASTContext &) const { return APValue(getValue()); }
void print(llvm::raw_ostream &OS) const {
// Can't use APFloat::print() since it appends a newline.
SmallVector<char, 16> Buffer;
- F.toString(Buffer);
+ getValue().toString(Buffer);
OS << Buffer;
}
std::string toDiagnosticString(const ASTContext &Ctx) const {
@@ -83,25 +100,62 @@ class Floating final {
return NameStr;
}
- unsigned bitWidth() const { return F.semanticsSizeInBits(F.getSemantics()); }
+ unsigned bitWidth() const {
+ return llvm::APFloatBase::semanticsSizeInBits(getSemantics());
+ }
+ unsigned numWords() const { return llvm::APInt::getNumWords(bitWidth()); }
+ bool singleWord() const {
+#if ALLOCATE_ALL
+ return false;
+#endif
+ return numWords() == 1;
+ }
+ static bool singleWord(const llvm::fltSemantics &Sem) {
+#if ALLOCATE_ALL
+ return false;
+#endif
+ return APInt::getNumWords(llvm::APFloatBase::getSizeInBits(Sem)) == 1;
+ }
+ const llvm::fltSemantics &getSemantics() const {
+ return llvm::APFloatBase::EnumToSemantics(Semantics);
+ }
+
+ void copy(const APFloat &F) {
+ if (singleWord()) {
+ Val = F.bitcastToAPInt().getZExtValue();
+ } else {
+ assert(Memory);
+ std::memcpy(Memory, F.bitcastToAPInt().getRawData(),
+ numWords() * sizeof(uint64_t));
+ }
+ }
+
+ void take(uint64_t *NewMemory) {
+ if (singleWord())
+ return;
+
+ if (Memory)
+ std::memcpy(NewMemory, Memory, numWords() * sizeof(uint64_t));
+ Memory = NewMemory;
+ }
bool isSigned() const { return true; }
- bool isNegative() const { return F.isNegative(); }
- bool isZero() const { return F.isZero(); }
- bool isNonZero() const { return F.isNonZero(); }
- bool isMin() const { return F.isSmallest(); }
- bool isMinusOne() const { return F.isExactlyValue(-1.0); }
- bool isNan() const { return F.isNaN(); }
- bool isSignaling() const { return F.isSignaling(); }
- bool isInf() const { return F.isInfinity(); }
- bool isFinite() const { return F.isFinite(); }
- bool isNormal() const { return F.isNormal(); }
- bool isDenormal() const { return F.isDenormal(); }
- llvm::FPClassTest classify() const { return F.classify(); }
- APFloat::fltCategory getCategory() const { return F.getCategory(); }
+ bool isNegative() const { return getValue().isNegative(); }
+ bool isZero() const { return getValue().isZero(); }
+ bool isNonZero() const { return getValue().isNonZero(); }
+ bool isMin() const { return getValue().isSmallest(); }
+ bool isMinusOne() const { return getValue().isExactlyValue(-1.0); }
+ bool isNan() const { return getValue().isNaN(); }
+ bool isSignaling() const { return getValue().isSignaling(); }
+ bool isInf() const { return getValue().isInfinity(); }
+ bool isFinite() const { return getValue().isFinite(); }
+ bool isNormal() const { return getValue().isNormal(); }
+ bool isDenormal() const { return getValue().isDenormal(); }
+ llvm::FPClassTest classify() const { return getValue().classify(); }
+ APFloat::fltCategory getCategory() const { return getValue().getCategory(); }
ComparisonCategoryResult compare(const Floating &RHS) const {
- llvm::APFloatBase::cmpResult CmpRes = F.compare(RHS.F);
+ llvm::APFloatBase::cmpResult CmpRes = getValue().compare(RHS.getValue());
switch (CmpRes) {
case llvm::APFloatBase::cmpLessThan:
return ComparisonCategoryResult::Less;
@@ -118,97 +172,130 @@ class Floating final {
static APFloat::opStatus fromIntegral(APSInt Val,
const llvm::fltSemantics &Sem,
llvm::RoundingMode RM,
- Floating &Result) {
+ Floating *Result) {
APFloat F = APFloat(Sem);
APFloat::opStatus Status = F.convertFromAPInt(Val, Val.isSigned(), RM);
- Result = Floating(F);
+ Result->copy(F);
return Status;
}
- static Floating bitcastFromMemory(const std::byte *Buff,
- const llvm::fltSemantics &Sem) {
+ static void bitcastFromMemory(const std::byte *Buff,
+ const llvm::fltSemantics &Sem,
+ Floating *Result) {
size_t Size = APFloat::semanticsSizeInBits(Sem);
llvm::APInt API(Size, true);
llvm::LoadIntFromMemory(API, (const uint8_t *)Buff, Size / 8);
-
- return Floating(APFloat(Sem, API));
+ Result->copy(APFloat(Sem, API));
}
void bitcastToMemory(std::byte *Buff) const {
- llvm::APInt API = F.bitcastToAPInt();
+ llvm::APInt API = getValue().bitcastToAPInt();
llvm::StoreIntToMemory(API, (uint8_t *)Buff, bitWidth() / 8);
}
// === Serialization support ===
size_t bytesToSerialize() const {
- return sizeof(llvm::fltSemantics *) +
- (APFloat::semanticsSizeInBits(F.getSemantics()) / 8);
+ return sizeof(Semantics) + (numWords() * sizeof(uint64_t));
}
void serialize(std::byte *Buff) const {
- // Semantics followed by an APInt.
- *reinterpret_cast<const llvm::fltSemantics **>(Buff) = &F.getSemantics();
-
- llvm::APInt API = F.bitcastToAPInt();
- llvm::StoreIntToMemory(API, (uint8_t *)(Buff + sizeof(void *)),
- bitWidth() / 8);
+ std::memcpy(Buff, &Semantics, sizeof(Semantics));
+ if (singleWord()) {
+ std::memcpy(Buff + sizeof(Semantics), &Val, sizeof(uint64_t));
+ } else {
+ std::memcpy(Buff + sizeof(Semantics), Memory,
+ numWords() * sizeof(uint64_t));
+ }
}
- static Floating deserialize(const std::byte *Buff) {
- const llvm::fltSemantics *Sem;
- std::memcpy((void *)&Sem, Buff, sizeof(void *));
- return bitcastFromMemory(Buff + sizeof(void *), *Sem);
+ static llvm::APFloatBase::Semantics
+ deserializeSemantics(const std::byte *Buff) {
+ return *reinterpret_cast<const llvm::APFloatBase::Semantics *>(Buff);
}
- static Floating abs(const Floating &F) {
- APFloat V = F.F;
- if (V.isNegative())
- V.changeSign();
- return Floating(V);
+ static void deserialize(const std::byte *Buff, Floating *Result) {
+ llvm::APFloatBase::Semantics Semantics;
+ std::memcpy(&Semantics, Buff, sizeof(Semantics));
+
+ unsigned BitWidth = llvm::APFloa...
[truncated]
|
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- clang/lib/AST/ByteCode/Compiler.cpp clang/lib/AST/ByteCode/Compiler.h clang/lib/AST/ByteCode/Descriptor.cpp clang/lib/AST/ByteCode/Disasm.cpp clang/lib/AST/ByteCode/Floating.h clang/lib/AST/ByteCode/Integral.h clang/lib/AST/ByteCode/IntegralAP.h clang/lib/AST/ByteCode/Interp.cpp clang/lib/AST/ByteCode/Interp.h clang/lib/AST/ByteCode/InterpBuiltin.cpp clang/lib/AST/ByteCode/InterpBuiltinBitCast.cpp clang/lib/AST/ByteCode/InterpState.h clang/lib/AST/ByteCode/PrimType.h clang/lib/AST/ByteCode/Program.h clang/test/AST/ByteCode/builtin-bit-cast-long-double.cpp clang/test/AST/ByteCode/builtin-functions.cppView the diff from clang-format here.diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index 66d3e6d79..c2ad387ed 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -2729,9 +2729,9 @@ inline bool DoShift(InterpState &S, CodePtr OpPC, LT &LHS, RT &RHS,
if (!S.noteUndefinedBehavior())
return false;
RHS = -RHS;
- return DoShift<LT, RT,
- Dir == ShiftDir::Left ? ShiftDir::Right : ShiftDir::Left>(
- S, OpPC, LHS, RHS, Result);
+ return DoShift < LT, RT,
+ Dir == ShiftDir::Left ? ShiftDir::Right
+ : ShiftDir::Left > (S, OpPC, LHS, RHS, Result);
}
if (!CheckShift<Dir>(S, OpPC, LHS, RHS, Bits))
@@ -2810,9 +2810,9 @@ inline bool DoShiftAP(InterpState &S, CodePtr OpPC, LT &LHS, RT &RHS,
if (!S.noteUndefinedBehavior())
return false;
RHS = -RHS;
- return DoShiftAP<LT, RT,
- Dir == ShiftDir::Left ? ShiftDir::Right : ShiftDir::Left>(
- S, OpPC, LHS, RHS, Result);
+ return DoShiftAP < LT, RT,
+ Dir == ShiftDir::Left ? ShiftDir::Right
+ : ShiftDir::Left > (S, OpPC, LHS, RHS, Result);
}
if (!CheckShift<Dir>(S, OpPC, LHS, RHS, Bits))
|
|
Formatting seems to be a difference in clang-format versions. I already formatted the patch locally. |
clang/lib/AST/ByteCode/Floating.h
Outdated
| uint64_t Val = 0; | ||
| uint64_t *Memory; | ||
| }; | ||
| llvm::APFloatBase::Semantics Semantics = |
There was a problem hiding this comment.
Why is the default half?
There was a problem hiding this comment.
That's just the first value in the enumeration.
There was a problem hiding this comment.
I wonder if it's better to leave it without a default initializer so if we ever add a new constructor, we get sanitizer diagnostics if we forget to initialize the field rather than have the wrong semantics? I don't feel strongly, but the default definitely surprised me here.
AaronBallman
approved these changes
Jun 17, 2025
tbaederr
added a commit
that referenced
this pull request
Jun 17, 2025
…g an allocator (#144246)" This reverts commit c66be28. This breaks the armv8-quick builder: https://lab.llvm.org/buildbot/#/builders/154/builds/17549
|
I see you reverted due to https://lab.llvm.org/buildbot/#/builders/154/builds/17549. In case you don't know, that builder is Armv8 32-bit. Unless you get reports from our other, 64-bit builders, it's probably an assumption about bitness. Let me know if you need any help with figuring it out. |
tbaederr
added a commit
to tbaederr/llvm-project
that referenced
this pull request
Jun 18, 2025
…ng an allocator (llvm#144246)" This reverts commit 57828fe.
fschlimb
pushed a commit
to fschlimb/llvm-project
that referenced
this pull request
Jun 18, 2025
…ocator (llvm#144246) Both `APInt` and `APFloat` will heap-allocate memory themselves using the system allocator when the size of their data exceeds 64 bits. This is why clang has `APNumericStorage`, which allocates its memory using an allocator (via `ASTContext`) instead. Calling `getValue()` on an ast node like that will then create a new `APInt`/`APFloat` , which will copy the data (in the `APFloat` case, we even copy it twice). That's sad but whatever. In the bytecode interpreter, we have a similar problem. Large integers and floating-point values are placement-new allocated into the `InterpStack` (or into the bytecode, which is a `vector<std::byte>`). When we then later interrupt interpretation, we don't run the destructor for all items on the stack, which means we leak the memory the `APInt`/`APFloat` (which backs the `IntegralAP`/`Floating` the interpreter uses). Fix this by using an approach similar to the one used in the AST. Add an allocator to `InterpState`, which is used for temporaries and local values. Those values will be freed at the end of interpretation. For global variables, we need to promote the values to global lifetime, which we do via `InitGlobal` and `FinishInitGlobal` ops. Interestingly, this results in a slight _improvement_ in compile times: https://llvm-compile-time-tracker.com/compare.php?from=6bfcdda9b1ddf0900f82f7e30cb5e3253a791d50&to=88d1d899127b408f0fb0f385c2c58e6283195049&stat=instructions:u (but don't ask me why). Fixes llvm#139012
fschlimb
pushed a commit
to fschlimb/llvm-project
that referenced
this pull request
Jun 18, 2025
…g an allocator (llvm#144246)" This reverts commit c66be28. This breaks the armv8-quick builder: https://lab.llvm.org/buildbot/#/builders/154/builds/17549
fschlimb
pushed a commit
to fschlimb/llvm-project
that referenced
this pull request
Jun 18, 2025
llvm#144676) …ng an allocator (llvm#144246)" This reverts commit 57828fe.
tbaederr
added a commit
to tbaederr/llvm-project
that referenced
this pull request
Jun 19, 2025
…ng an allocator (llvm#144246)" This reverts commit 57828fe.
|
So after the fixup this failed again: @antmox please reproduce this on Armv8 32-bit and help @tbaederr figure out the problem. If you're lucky, getting a decent callstack will be enough to figure it out. Another approach is to scan the changes for any use of |
|
This only reproduces if we build it with clang (19.1.7, but might not be version specific). I added a debug trap in place of the assert and got a better traceback: So either it's a clang bug or its undefined behaviour, I bet the latter but this backtrace isn't much help. Trying to get a debug build, proving difficult due to memory limitations. |
|
Short of cross-compiling, a debug build isn't happening. I know so far is that the last thing it attempts to do is construct an APInt with: We need to find where that value comes from. |
|
Can you try applying diff --git i/clang/lib/AST/ByteCode/IntegralAP.h w/clang/lib/AST/ByteCode/IntegralAP.h
index 0280ea072ca9..4c1fb61e3e15 100644
--- i/clang/lib/AST/ByteCode/IntegralAP.h
+++ w/clang/lib/AST/ByteCode/IntegralAP.h
@@ -92,7 +92,10 @@ public:
// Constructors.
IntegralAP() = default;
- IntegralAP(unsigned BitWidth) : BitWidth(BitWidth) {}
+ IntegralAP(unsigned BitWidth) : Val(0), BitWidth(BitWidth) {
+ assert(singleWord());
+ Val = 0;
+ }
IntegralAP(uint64_t *Memory, unsigned BitWidth)
: Memory(Memory), BitWidth(BitWidth) {}
IntegralAP(const APInt &V) : BitWidth(V.getBitWidth()) {(or the equivalent) and re-checking? In the single-word case, we're not explicitly setting (I wish ubsan would catch reads from inactive union members but alas) |
|
When applied to 7c15edb, this fixes the problem and passes check-clang. Reland in whatever way you prefer (no need for extra review), as you know, the bot will rapidly find out if there are problems. I just ask that you include a note in the commit message describing what was added from last time it was landed, in case we have an issue like this in future. |
tbaederr
added a commit
that referenced
this pull request
Jun 20, 2025
#145014) …types usi… (#144676)" This reverts commit 68471d2. IntegralAP contains a union: union { uint64_t *Memory = nullptr; uint64_t Val; }; On 64bit systems, both Memory and Val have the same size. However, on 32 bit system, Val is 64bit and Memory only 32bit. Which means the default initializer for Memory will only zero half of Val. We fixed this by zero-initializing Val explicitly in the IntegralAP(unsigned BitWidth) constructor. See also the discussion in #144246
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jun 20, 2025
…d Floating … (#145014) …types usi… (#144676)" This reverts commit 68471d2. IntegralAP contains a union: union { uint64_t *Memory = nullptr; uint64_t Val; }; On 64bit systems, both Memory and Val have the same size. However, on 32 bit system, Val is 64bit and Memory only 32bit. Which means the default initializer for Memory will only zero half of Val. We fixed this by zero-initializing Val explicitly in the IntegralAP(unsigned BitWidth) constructor. See also the discussion in llvm/llvm-project#144246
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.
Both
APIntandAPFloatwill heap-allocate memory themselves using the system allocator when the size of their data exceeds 64 bits.This is why clang has
APNumericStorage, which allocates its memory using an allocator (viaASTContext) instead. CallinggetValue()on an ast node like that will then create a newAPInt/APFloat, which will copy the data (in theAPFloatcase, we even copy it twice). That's sad but whatever.In the bytecode interpreter, we have a similar problem. Large integers and floating-point values are placement-new allocated into the
InterpStack(or into the bytecode, which is avector<std::byte>). When we then later interrupt interpretation, we don't run the destructor for all items on the stack, which means we leak the memory theAPInt/APFloat(which backs theIntegralAP/Floatingthe interpreter uses).Fix this by using an approach similar to the one used in the AST. Add an allocator to
InterpState, which is used for temporaries and local values. Those values will be freed at the end of interpretation. For global variables, we need to promote the values to global lifetime, which we do viaInitGlobalandFinishInitGlobalops.Interestingly, this results in a slight improvement in compile times: https://llvm-compile-time-tracker.com/compare.php?from=6bfcdda9b1ddf0900f82f7e30cb5e3253a791d50&to=88d1d899127b408f0fb0f385c2c58e6283195049&stat=instructions:u (but don't ask me why).
Fixes #139012