Revert "Improve stack usage to increase recursive initialization depth" #89006
Conversation
This file contains 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: Vitaly Buka (vitalybuka) ChangesReverts llvm/llvm-project#88546 Leak and performance regression. Full diff: https://github.com/llvm/llvm-project/pull/89006.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4aedfafcb26aea..3752b6ce157600 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -203,12 +203,6 @@ Non-comprehensive list of changes in this release
- ``__typeof_unqual__`` is available in all C modes as an extension, which behaves
like ``typeof_unqual`` from C23, similar to ``__typeof__`` and ``typeof``.
-- Improved stack usage with C++ initialization code. This allows significantly
- more levels of recursive initialization before reaching stack exhaustion
- limits. This will positively impact recursive template instantiation code,
- but should also reduce memory overhead for initializations in general.
- Fixes #GH88330
-
New Compiler Flags
------------------
- ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and
diff --git a/clang/include/clang/Sema/Initialization.h b/clang/include/clang/Sema/Initialization.h
index 1ceacf0f49f568..2072cd8d1c3ef8 100644
--- a/clang/include/clang/Sema/Initialization.h
+++ b/clang/include/clang/Sema/Initialization.h
@@ -1134,7 +1134,7 @@ class InitializationSequence {
OverloadingResult FailedOverloadResult;
/// The candidate set created when initialization failed.
- std::unique_ptr<OverloadCandidateSet> FailedCandidateSet;
+ OverloadCandidateSet FailedCandidateSet;
/// The incomplete type that caused a failure.
QualType FailedIncompleteType;
@@ -1403,9 +1403,7 @@ class InitializationSequence {
/// Retrieve a reference to the candidate set when overload
/// resolution fails.
OverloadCandidateSet &getFailedCandidateSet() {
- assert(FailedCandidateSet &&
- "this should have been allocated in the constructor!");
- return *FailedCandidateSet;
+ return FailedCandidateSet;
}
/// Get the overloading result, for when the initialization
diff --git a/clang/include/clang/Sema/Overload.h b/clang/include/clang/Sema/Overload.h
index e6f88bbf7c4f47..76311b00d2fc58 100644
--- a/clang/include/clang/Sema/Overload.h
+++ b/clang/include/clang/Sema/Overload.h
@@ -37,7 +37,6 @@
#include <cassert>
#include <cstddef>
#include <cstdint>
-#include <memory>
#include <utility>
namespace clang {
@@ -875,8 +874,7 @@ class Sema;
ConversionFixItGenerator Fix;
/// Viable - True to indicate that this overload candidate is viable.
- LLVM_PREFERRED_TYPE(bool)
- unsigned Viable : 1;
+ bool Viable : 1;
/// Whether this candidate is the best viable function, or tied for being
/// the best viable function.
@@ -885,14 +883,12 @@ class Sema;
/// was part of the ambiguity kernel: the minimal non-empty set of viable
/// candidates such that all elements of the ambiguity kernel are better
/// than all viable candidates not in the ambiguity kernel.
- LLVM_PREFERRED_TYPE(bool)
- unsigned Best : 1;
+ bool Best : 1;
/// IsSurrogate - True to indicate that this candidate is a
/// surrogate for a conversion to a function pointer or reference
/// (C++ [over.call.object]).
- LLVM_PREFERRED_TYPE(bool)
- unsigned IsSurrogate : 1;
+ bool IsSurrogate : 1;
/// IgnoreObjectArgument - True to indicate that the first
/// argument's conversion, which for this function represents the
@@ -901,20 +897,18 @@ class Sema;
/// implicit object argument is just a placeholder) or a
/// non-static member function when the call doesn't have an
/// object argument.
- LLVM_PREFERRED_TYPE(bool)
- unsigned IgnoreObjectArgument : 1;
+ bool IgnoreObjectArgument : 1;
/// True if the candidate was found using ADL.
- LLVM_PREFERRED_TYPE(CallExpr::ADLCallKind)
- unsigned IsADLCandidate : 1;
+ CallExpr::ADLCallKind IsADLCandidate : 1;
/// Whether this is a rewritten candidate, and if so, of what kind?
LLVM_PREFERRED_TYPE(OverloadCandidateRewriteKind)
unsigned RewriteKind : 2;
/// FailureKind - The reason why this candidate is not viable.
- LLVM_PREFERRED_TYPE(OverloadFailureKind)
- unsigned FailureKind : 5;
+ /// Actually an OverloadFailureKind.
+ unsigned char FailureKind;
/// The number of call arguments that were explicitly provided,
/// to be used while performing partial ordering of function templates.
@@ -978,9 +972,7 @@ class Sema;
private:
friend class OverloadCandidateSet;
OverloadCandidate()
- : IsSurrogate(false),
- IsADLCandidate(static_cast<unsigned>(CallExpr::NotADL)),
- RewriteKind(CRK_None) {}
+ : IsSurrogate(false), IsADLCandidate(CallExpr::NotADL), RewriteKind(CRK_None) {}
};
/// OverloadCandidateSet - A set of overload candidates, used in C++
@@ -1078,16 +1070,51 @@ class Sema;
};
private:
- SmallVector<OverloadCandidate, 4> Candidates;
- llvm::SmallPtrSet<uintptr_t, 4> Functions;
+ SmallVector<OverloadCandidate, 16> Candidates;
+ llvm::SmallPtrSet<uintptr_t, 16> Functions;
+
+ // Allocator for ConversionSequenceLists. We store the first few of these
+ // inline to avoid allocation for small sets.
+ llvm::BumpPtrAllocator SlabAllocator;
SourceLocation Loc;
CandidateSetKind Kind;
OperatorRewriteInfo RewriteInfo;
+ constexpr static unsigned NumInlineBytes =
+ 24 * sizeof(ImplicitConversionSequence);
+ unsigned NumInlineBytesUsed = 0;
+ alignas(void *) char InlineSpace[NumInlineBytes];
+
// Address space of the object being constructed.
LangAS DestAS = LangAS::Default;
+ /// If we have space, allocates from inline storage. Otherwise, allocates
+ /// from the slab allocator.
+ /// FIXME: It would probably be nice to have a SmallBumpPtrAllocator
+ /// instead.
+ /// FIXME: Now that this only allocates ImplicitConversionSequences, do we
+ /// want to un-generalize this?
+ template <typename T>
+ T *slabAllocate(unsigned N) {
+ // It's simpler if this doesn't need to consider alignment.
+ static_assert(alignof(T) == alignof(void *),
+ "Only works for pointer-aligned types.");
+ static_assert(std::is_trivial<T>::value ||
+ std::is_same<ImplicitConversionSequence, T>::value,
+ "Add destruction logic to OverloadCandidateSet::clear().");
+
+ unsigned NBytes = sizeof(T) * N;
+ if (NBytes > NumInlineBytes - NumInlineBytesUsed)
+ return SlabAllocator.Allocate<T>(N);
+ char *FreeSpaceStart = InlineSpace + NumInlineBytesUsed;
+ assert(uintptr_t(FreeSpaceStart) % alignof(void *) == 0 &&
+ "Misaligned storage!");
+
+ NumInlineBytesUsed += NBytes;
+ return reinterpret_cast<T *>(FreeSpaceStart);
+ }
+
void destroyCandidates();
public:
@@ -1136,7 +1163,12 @@ class Sema;
ConversionSequenceList
allocateConversionSequences(unsigned NumConversions) {
ImplicitConversionSequence *Conversions =
- new ImplicitConversionSequence[NumConversions];
+ slabAllocate<ImplicitConversionSequence>(NumConversions);
+
+ // Construct the new objects.
+ for (unsigned I = 0; I != NumConversions; ++I)
+ new (&Conversions[I]) ImplicitConversionSequence();
+
return ConversionSequenceList(Conversions, NumConversions);
}
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 791c0b6e6df23e..fb7a80ab02846c 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -6114,8 +6114,7 @@ InitializationSequence::InitializationSequence(
Sema &S, const InitializedEntity &Entity, const InitializationKind &Kind,
MultiExprArg Args, bool TopLevelOfInitList, bool TreatUnavailableAsInvalid)
: FailedOverloadResult(OR_Success),
- FailedCandidateSet(new OverloadCandidateSet(
- Kind.getLocation(), OverloadCandidateSet::CSK_Normal)) {
+ FailedCandidateSet(Kind.getLocation(), OverloadCandidateSet::CSK_Normal) {
InitializeFrom(S, Entity, Kind, Args, TopLevelOfInitList,
TreatUnavailableAsInvalid);
}
@@ -9736,7 +9735,7 @@ bool InitializationSequence::Diagnose(Sema &S,
switch (FailedOverloadResult) {
case OR_Ambiguous:
- FailedCandidateSet->NoteCandidates(
+ FailedCandidateSet.NoteCandidates(
PartialDiagnosticAt(
Kind.getLocation(),
Failure == FK_UserConversionOverloadFailed
@@ -9750,8 +9749,7 @@ bool InitializationSequence::Diagnose(Sema &S,
break;
case OR_No_Viable_Function: {
- auto Cands =
- FailedCandidateSet->CompleteCandidates(S, OCD_AllCandidates, Args);
+ auto Cands = FailedCandidateSet.CompleteCandidates(S, OCD_AllCandidates, Args);
if (!S.RequireCompleteType(Kind.getLocation(),
DestType.getNonReferenceType(),
diag::err_typecheck_nonviable_condition_incomplete,
@@ -9761,13 +9759,13 @@ bool InitializationSequence::Diagnose(Sema &S,
<< OnlyArg->getType() << Args[0]->getSourceRange()
<< DestType.getNonReferenceType();
- FailedCandidateSet->NoteCandidates(S, Args, Cands);
+ FailedCandidateSet.NoteCandidates(S, Args, Cands);
break;
}
case OR_Deleted: {
OverloadCandidateSet::iterator Best;
- OverloadingResult Ovl =
- FailedCandidateSet->BestViableFunction(S, Kind.getLocation(), Best);
+ OverloadingResult Ovl
+ = FailedCandidateSet.BestViableFunction(S, Kind.getLocation(), Best);
StringLiteral *Msg = Best->Function->getDeletedMessage();
S.Diag(Kind.getLocation(), diag::err_typecheck_deleted_function)
@@ -9951,7 +9949,7 @@ bool InitializationSequence::Diagnose(Sema &S,
// bad.
switch (FailedOverloadResult) {
case OR_Ambiguous:
- FailedCandidateSet->NoteCandidates(
+ FailedCandidateSet.NoteCandidates(
PartialDiagnosticAt(Kind.getLocation(),
S.PDiag(diag::err_ovl_ambiguous_init)
<< DestType << ArgsRange),
@@ -10005,7 +10003,7 @@ bool InitializationSequence::Diagnose(Sema &S,
break;
}
- FailedCandidateSet->NoteCandidates(
+ FailedCandidateSet.NoteCandidates(
PartialDiagnosticAt(
Kind.getLocation(),
S.PDiag(diag::err_ovl_no_viable_function_in_init)
@@ -10015,8 +10013,8 @@ bool InitializationSequence::Diagnose(Sema &S,
case OR_Deleted: {
OverloadCandidateSet::iterator Best;
- OverloadingResult Ovl =
- FailedCandidateSet->BestViableFunction(S, Kind.getLocation(), Best);
+ OverloadingResult Ovl
+ = FailedCandidateSet.BestViableFunction(S, Kind.getLocation(), Best);
if (Ovl != OR_Deleted) {
S.Diag(Kind.getLocation(), diag::err_ovl_deleted_init)
<< DestType << ArgsRange;
@@ -10095,8 +10093,8 @@ bool InitializationSequence::Diagnose(Sema &S,
S.Diag(Kind.getLocation(), diag::err_selected_explicit_constructor)
<< Args[0]->getSourceRange();
OverloadCandidateSet::iterator Best;
- OverloadingResult Ovl =
- FailedCandidateSet->BestViableFunction(S, Kind.getLocation(), Best);
+ OverloadingResult Ovl
+ = FailedCandidateSet.BestViableFunction(S, Kind.getLocation(), Best);
(void)Ovl;
assert(Ovl == OR_Success && "Inconsistent overload resolution");
CXXConstructorDecl *CtorDecl = cast<CXXConstructorDecl>(Best->Function);
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index bcde0d86cf10fd..227ef564ba3e08 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -1057,7 +1057,8 @@ bool OverloadCandidateSet::OperatorRewriteInfo::shouldAddReversed(
void OverloadCandidateSet::destroyCandidates() {
for (iterator i = begin(), e = end(); i != e; ++i) {
- delete[] i->Conversions.data();
+ for (auto &C : i->Conversions)
+ C.~ImplicitConversionSequence();
if (!i->Viable && i->FailureKind == ovl_fail_bad_deduction)
i->DeductionFailure.Destroy();
}
@@ -1065,6 +1066,8 @@ void OverloadCandidateSet::destroyCandidates() {
void OverloadCandidateSet::clear(CandidateSetKind CSK) {
destroyCandidates();
+ SlabAllocator.Reset();
+ NumInlineBytesUsed = 0;
Candidates.clear();
Functions.clear();
Kind = CSK;
@@ -6980,7 +6983,7 @@ void Sema::AddOverloadCandidate(
Candidate.RewriteKind =
CandidateSet.getRewriteInfo().getRewriteKind(Function, PO);
Candidate.IsSurrogate = false;
- Candidate.IsADLCandidate = static_cast<unsigned>(IsADLCandidate);
+ Candidate.IsADLCandidate = IsADLCandidate;
Candidate.IgnoreObjectArgument = false;
Candidate.ExplicitCallArguments = Args.size();
@@ -7812,7 +7815,7 @@ void Sema::AddTemplateOverloadCandidate(
Candidate.RewriteKind =
CandidateSet.getRewriteInfo().getRewriteKind(Candidate.Function, PO);
Candidate.IsSurrogate = false;
- Candidate.IsADLCandidate = static_cast<unsigned>(IsADLCandidate);
+ Candidate.IsADLCandidate = IsADLCandidate;
// Ignore the object argument if there is one, since we don't have an object
// type.
Candidate.IgnoreObjectArgument =
@@ -14122,8 +14125,7 @@ static ExprResult FinishOverloadedCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
return ExprError();
return SemaRef.BuildResolvedCallExpr(
Res.get(), FDecl, LParenLoc, Args, RParenLoc, ExecConfig,
- /*IsExecConfig=*/false,
- static_cast<CallExpr::ADLCallKind>((*Best)->IsADLCandidate));
+ /*IsExecConfig=*/false, (*Best)->IsADLCandidate);
}
case OR_No_Viable_Function: {
@@ -14182,8 +14184,7 @@ static ExprResult FinishOverloadedCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
return ExprError();
return SemaRef.BuildResolvedCallExpr(
Res.get(), FDecl, LParenLoc, Args, RParenLoc, ExecConfig,
- /*IsExecConfig=*/false,
- static_cast<CallExpr::ADLCallKind>((*Best)->IsADLCandidate));
+ /*IsExecConfig=*/false, (*Best)->IsADLCandidate);
}
}
@@ -14490,8 +14491,7 @@ Sema::CreateOverloadedUnaryOp(SourceLocation OpLoc, UnaryOperatorKind Opc,
Args[0] = Input;
CallExpr *TheCall = CXXOperatorCallExpr::Create(
Context, Op, FnExpr.get(), ArgsArray, ResultTy, VK, OpLoc,
- CurFPFeatureOverrides(),
- static_cast<CallExpr::ADLCallKind>(Best->IsADLCandidate));
+ CurFPFeatureOverrides(), Best->IsADLCandidate);
if (CheckCallReturnType(FnDecl->getReturnType(), OpLoc, TheCall, FnDecl))
return ExprError();
@@ -14909,8 +14909,7 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation OpLoc,
// members; CodeGen should take care not to emit the this pointer.
TheCall = CXXOperatorCallExpr::Create(
Context, ChosenOp, FnExpr.get(), Args, ResultTy, VK, OpLoc,
- CurFPFeatureOverrides(),
- static_cast<CallExpr::ADLCallKind>(Best->IsADLCandidate));
+ CurFPFeatureOverrides(), Best->IsADLCandidate);
if (const auto *Method = dyn_cast<CXXMethodDecl>(FnDecl);
Method && Method->isImplicitObjectMemberFunction()) {
|
You can test this locally with the following command:git-clang-format --diff 8c9d814b66f7df274de41225575817188fbeed4f 1d59298cd9ed21e1ac860d64f965514a577f45bb -- clang/include/clang/Sema/Initialization.h clang/include/clang/Sema/Overload.h clang/lib/Sema/SemaInit.cpp clang/lib/Sema/SemaOverload.cppView the diff from clang-format here.diff --git a/clang/include/clang/Sema/Initialization.h b/clang/include/clang/Sema/Initialization.h
index 2072cd8d1c..0cae6d353d 100644
--- a/clang/include/clang/Sema/Initialization.h
+++ b/clang/include/clang/Sema/Initialization.h
@@ -1402,9 +1402,7 @@ public:
/// Retrieve a reference to the candidate set when overload
/// resolution fails.
- OverloadCandidateSet &getFailedCandidateSet() {
- return FailedCandidateSet;
- }
+ OverloadCandidateSet &getFailedCandidateSet() { return FailedCandidateSet; }
/// Get the overloading result, for when the initialization
/// sequence failed due to a bad overload.
diff --git a/clang/include/clang/Sema/Overload.h b/clang/include/clang/Sema/Overload.h
index 76311b00d2..a49cc6778d 100644
--- a/clang/include/clang/Sema/Overload.h
+++ b/clang/include/clang/Sema/Overload.h
@@ -972,7 +972,8 @@ class Sema;
private:
friend class OverloadCandidateSet;
OverloadCandidate()
- : IsSurrogate(false), IsADLCandidate(CallExpr::NotADL), RewriteKind(CRK_None) {}
+ : IsSurrogate(false), IsADLCandidate(CallExpr::NotADL),
+ RewriteKind(CRK_None) {}
};
/// OverloadCandidateSet - A set of overload candidates, used in C++
@@ -1095,8 +1096,7 @@ class Sema;
/// instead.
/// FIXME: Now that this only allocates ImplicitConversionSequences, do we
/// want to un-generalize this?
- template <typename T>
- T *slabAllocate(unsigned N) {
+ template <typename T> T *slabAllocate(unsigned N) {
// It's simpler if this doesn't need to consider alignment.
static_assert(alignof(T) == alignof(void *),
"Only works for pointer-aligned types.");
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index fb7a80ab02..6afcc56c73 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -9749,7 +9749,8 @@ bool InitializationSequence::Diagnose(Sema &S,
break;
case OR_No_Viable_Function: {
- auto Cands = FailedCandidateSet.CompleteCandidates(S, OCD_AllCandidates, Args);
+ auto Cands =
+ FailedCandidateSet.CompleteCandidates(S, OCD_AllCandidates, Args);
if (!S.RequireCompleteType(Kind.getLocation(),
DestType.getNonReferenceType(),
diag::err_typecheck_nonviable_condition_incomplete,
@@ -9764,8 +9765,8 @@ bool InitializationSequence::Diagnose(Sema &S,
}
case OR_Deleted: {
OverloadCandidateSet::iterator Best;
- OverloadingResult Ovl
- = FailedCandidateSet.BestViableFunction(S, Kind.getLocation(), Best);
+ OverloadingResult Ovl =
+ FailedCandidateSet.BestViableFunction(S, Kind.getLocation(), Best);
StringLiteral *Msg = Best->Function->getDeletedMessage();
S.Diag(Kind.getLocation(), diag::err_typecheck_deleted_function)
@@ -10013,8 +10014,8 @@ bool InitializationSequence::Diagnose(Sema &S,
case OR_Deleted: {
OverloadCandidateSet::iterator Best;
- OverloadingResult Ovl
- = FailedCandidateSet.BestViableFunction(S, Kind.getLocation(), Best);
+ OverloadingResult Ovl =
+ FailedCandidateSet.BestViableFunction(S, Kind.getLocation(), Best);
if (Ovl != OR_Deleted) {
S.Diag(Kind.getLocation(), diag::err_ovl_deleted_init)
<< DestType << ArgsRange;
@@ -10093,8 +10094,8 @@ bool InitializationSequence::Diagnose(Sema &S,
S.Diag(Kind.getLocation(), diag::err_selected_explicit_constructor)
<< Args[0]->getSourceRange();
OverloadCandidateSet::iterator Best;
- OverloadingResult Ovl
- = FailedCandidateSet.BestViableFunction(S, Kind.getLocation(), Best);
+ OverloadingResult Ovl =
+ FailedCandidateSet.BestViableFunction(S, Kind.getLocation(), Best);
(void)Ovl;
assert(Ovl == OR_Success && "Inconsistent overload resolution");
CXXConstructorDecl *CtorDecl = cast<CXXConstructorDecl>(Best->Function);
|
fmayer
approved these changes
Apr 17, 2024
fmayer
reviewed
Apr 17, 2024
vitalybuka
force-pushed
the
revert-88546-aballman-stack-size
branch
from
b9f0b0e
to
1d59298
Compare
vitalybuka
added
the
skip-precommit-approval
Done. Reverted to github generated 1d59298 |
|
Thank you for the revert! |
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.
Reverts #88546
Leak and performance regression.
Details in #88546