[clang] Track final substitution for Subst* AST nodes #132748
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang Author: Matheus Izvekov (mizvekov) ChangesThis patch re-adds Subst* nodes for 'Final' substitutions, adding a bit to these nodes to differentiate them from non-Final ones, which for example helps track forward progress in resugaring. This was originally changed when we added support for sugared substitutions in the template instantiator (326feaa), and later applied to NTTP and default argument substitution (ab11408) and type alias templates (7f78f99). This kind of instantiation is favored for entities which we don't track specializations, like the aforementioned cases because:
While the node could have been still useful, its removal helped offset the extra costs of the sugared instantiation. Otherwise, adding them would also necessitate tracking which nodes don't need to be resugared later, which this patch does. Adding the node back does add a little bit of performance penalty on itself: 0.25% on stage2-clang. And this will make resugaring a little bit more expensive later when that's upstreamed, since there are more AST nodes to walk, and more AST nodes to produce. Before: After: Which on the current implementation and applications of resugaring transform, is an extra 0.24% on stage2-clang. Some upstream users reported desirability of keeping these Subst* nodes in all cases, and we had added a special frontend flag to control its removal, which this patch now also removes. Patch is 50.78 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/132748.diff 21 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 1f7c75559e1e9..2ef8112225767 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -1795,10 +1795,10 @@ class ASTContext : public RefCountedBase<ASTContext> {
QualType Wrapped, QualType Contained,
const HLSLAttributedResourceType::Attributes &Attrs);
- QualType
- getSubstTemplateTypeParmType(QualType Replacement, Decl *AssociatedDecl,
- unsigned Index,
- std::optional<unsigned> PackIndex) const;
+ QualType getSubstTemplateTypeParmType(QualType Replacement,
+ Decl *AssociatedDecl, unsigned Index,
+ std::optional<unsigned> PackIndex,
+ bool Final) const;
QualType getSubstTemplateTypeParmPackType(Decl *AssociatedDecl,
unsigned Index, bool Final,
const TemplateArgument &ArgPack);
@@ -2396,10 +2396,11 @@ class ASTContext : public RefCountedBase<ASTContext> {
const IdentifierInfo *Name) const;
TemplateName getDependentTemplateName(NestedNameSpecifier *NNS,
OverloadedOperatorKind Operator) const;
- TemplateName
- getSubstTemplateTemplateParm(TemplateName replacement, Decl *AssociatedDecl,
- unsigned Index,
- std::optional<unsigned> PackIndex) const;
+ TemplateName getSubstTemplateTemplateParm(TemplateName replacement,
+ Decl *AssociatedDecl,
+ unsigned Index,
+ std::optional<unsigned> PackIndex,
+ bool Final) const;
TemplateName getSubstTemplateTemplateParmPack(const TemplateArgument &ArgPack,
Decl *AssociatedDecl,
unsigned Index,
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index 223d74993e9e6..028ee82718d50 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -4514,7 +4514,9 @@ class SubstNonTypeTemplateParmExpr : public Expr {
llvm::PointerIntPair<Decl *, 1, bool> AssociatedDeclAndRef;
unsigned Index : 15;
- unsigned PackIndex : 16;
+ unsigned PackIndex : 15;
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned Final : 1;
explicit SubstNonTypeTemplateParmExpr(EmptyShell Empty)
: Expr(SubstNonTypeTemplateParmExprClass, Empty) {}
@@ -4523,11 +4525,12 @@ class SubstNonTypeTemplateParmExpr : public Expr {
SubstNonTypeTemplateParmExpr(QualType Ty, ExprValueKind ValueKind,
SourceLocation Loc, Expr *Replacement,
Decl *AssociatedDecl, unsigned Index,
- std::optional<unsigned> PackIndex, bool RefParam)
+ std::optional<unsigned> PackIndex, bool RefParam,
+ bool Final)
: Expr(SubstNonTypeTemplateParmExprClass, Ty, ValueKind, OK_Ordinary),
Replacement(Replacement),
AssociatedDeclAndRef(AssociatedDecl, RefParam), Index(Index),
- PackIndex(PackIndex ? *PackIndex + 1 : 0) {
+ PackIndex(PackIndex ? *PackIndex + 1 : 0), Final(Final) {
assert(AssociatedDecl != nullptr);
SubstNonTypeTemplateParmExprBits.NameLoc = Loc;
setDependence(computeDependence(this));
@@ -4555,6 +4558,10 @@ class SubstNonTypeTemplateParmExpr : public Expr {
return PackIndex - 1;
}
+ // This substitution is Final, which means the substitution is fully
+ // sugared: it doesn't need to be resugared later.
+ bool getFinal() const { return Final; }
+
NonTypeTemplateParmDecl *getParameter() const;
bool isReferenceParameter() const { return AssociatedDeclAndRef.getInt(); }
@@ -4598,7 +4605,10 @@ class SubstNonTypeTemplateParmPackExpr : public Expr {
const TemplateArgument *Arguments;
/// The number of template arguments in \c Arguments.
- unsigned NumArguments : 16;
+ unsigned NumArguments : 15;
+
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned Final : 1;
unsigned Index : 16;
@@ -4612,7 +4622,8 @@ class SubstNonTypeTemplateParmPackExpr : public Expr {
SubstNonTypeTemplateParmPackExpr(QualType T, ExprValueKind ValueKind,
SourceLocation NameLoc,
const TemplateArgument &ArgPack,
- Decl *AssociatedDecl, unsigned Index);
+ Decl *AssociatedDecl, unsigned Index,
+ bool Final);
/// A template-like entity which owns the whole pattern being substituted.
/// This will own a set of template parameters.
@@ -4622,6 +4633,10 @@ class SubstNonTypeTemplateParmPackExpr : public Expr {
/// This should match the result of `getParameterPack()->getIndex()`.
unsigned getIndex() const { return Index; }
+ // This substitution will be Final, which means the substitution will be fully
+ // sugared: it doesn't need to be resugared later.
+ bool getFinal() const { return Final; }
+
/// Retrieve the non-type template parameter pack being substituted.
NonTypeTemplateParmDecl *getParameterPack() const;
diff --git a/clang/include/clang/AST/PropertiesBase.td b/clang/include/clang/AST/PropertiesBase.td
index 5f3a885832e2e..416914db2f7c8 100644
--- a/clang/include/clang/AST/PropertiesBase.td
+++ b/clang/include/clang/AST/PropertiesBase.td
@@ -729,8 +729,9 @@ let Class = PropertyTypeCase<TemplateName, "SubstTemplateTemplateParm"> in {
def : Property<"packIndex", Optional<UInt32>> {
let Read = [{ parm->getPackIndex() }];
}
+ def : Property<"final", Bool> { let Read = [{ parm->getFinal() }]; }
def : Creator<[{
- return ctx.getSubstTemplateTemplateParm(replacement, associatedDecl, index, packIndex);
+ return ctx.getSubstTemplateTemplateParm(replacement, associatedDecl, index, packIndex, final);
}]>;
}
let Class = PropertyTypeCase<TemplateName, "SubstTemplateTemplateParmPack"> in {
diff --git a/clang/include/clang/AST/TemplateName.h b/clang/include/clang/AST/TemplateName.h
index ce97f834bfc1d..313802502f818 100644
--- a/clang/include/clang/AST/TemplateName.h
+++ b/clang/include/clang/AST/TemplateName.h
@@ -413,9 +413,11 @@ class SubstTemplateTemplateParmStorage
SubstTemplateTemplateParmStorage(TemplateName Replacement,
Decl *AssociatedDecl, unsigned Index,
- std::optional<unsigned> PackIndex)
+ std::optional<unsigned> PackIndex,
+ bool Final)
: UncommonTemplateNameStorage(SubstTemplateTemplateParm, Index,
- PackIndex ? *PackIndex + 1 : 0),
+ ((PackIndex ? *PackIndex + 1 : 0) << 1) |
+ Final),
Replacement(Replacement), AssociatedDecl(AssociatedDecl) {
assert(AssociatedDecl != nullptr);
}
@@ -429,10 +431,15 @@ class SubstTemplateTemplateParmStorage
/// This should match the result of `getParameter()->getIndex()`.
unsigned getIndex() const { return Bits.Index; }
+ // This substitution is Final, which means the substitution is fully
+ // sugared: it doesn't need to be resugared later.
+ bool getFinal() const { return Bits.Data & 1; }
+
std::optional<unsigned> getPackIndex() const {
- if (Bits.Data == 0)
+ auto Data = Bits.Data >> 1;
+ if (Data == 0)
return std::nullopt;
- return Bits.Data - 1;
+ return Data - 1;
}
TemplateTemplateParmDecl *getParameter() const;
@@ -442,7 +449,7 @@ class SubstTemplateTemplateParmStorage
static void Profile(llvm::FoldingSetNodeID &ID, TemplateName Replacement,
Decl *AssociatedDecl, unsigned Index,
- std::optional<unsigned> PackIndex);
+ std::optional<unsigned> PackIndex, bool Final);
};
class DeducedTemplateStorage : public UncommonTemplateNameStorage,
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 3b80f962a3bc6..16fde176fb909 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -2158,12 +2158,15 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
// The index of the template parameter this substitution represents.
unsigned Index : 15;
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned Final : 1;
+
/// Represents the index within a pack if this represents a substitution
/// from a pack expansion. This index starts at the end of the pack and
/// increments towards the beginning.
/// Positive non-zero number represents the index + 1.
/// Zero means this is not substituted from an expansion.
- unsigned PackIndex : 16;
+ unsigned PackIndex : 15;
};
class SubstTemplateTypeParmPackTypeBitfields {
@@ -6384,7 +6387,8 @@ class SubstTemplateTypeParmType final
Decl *AssociatedDecl;
SubstTemplateTypeParmType(QualType Replacement, Decl *AssociatedDecl,
- unsigned Index, std::optional<unsigned> PackIndex);
+ unsigned Index, std::optional<unsigned> PackIndex,
+ bool Final);
public:
/// Gets the type that was substituted for the template
@@ -6407,6 +6411,10 @@ class SubstTemplateTypeParmType final
/// This should match the result of `getReplacedParameter()->getIndex()`.
unsigned getIndex() const { return SubstTemplateTypeParmTypeBits.Index; }
+ // This substitution is Final, which means the substitution is fully
+ // sugared: it doesn't need to be resugared later.
+ unsigned getFinal() const { return SubstTemplateTypeParmTypeBits.Final; }
+
std::optional<unsigned> getPackIndex() const {
if (SubstTemplateTypeParmTypeBits.PackIndex == 0)
return std::nullopt;
@@ -6418,17 +6426,12 @@ class SubstTemplateTypeParmType final
void Profile(llvm::FoldingSetNodeID &ID) {
Profile(ID, getReplacementType(), getAssociatedDecl(), getIndex(),
- getPackIndex());
+ getPackIndex(), getFinal());
}
static void Profile(llvm::FoldingSetNodeID &ID, QualType Replacement,
const Decl *AssociatedDecl, unsigned Index,
- std::optional<unsigned> PackIndex) {
- Replacement.Profile(ID);
- ID.AddPointer(AssociatedDecl);
- ID.AddInteger(Index);
- ID.AddInteger(PackIndex ? *PackIndex - 1 : 0);
- }
+ std::optional<unsigned> PackIndex, bool Final);
static bool classof(const Type *T) {
return T->getTypeClass() == SubstTemplateTypeParm;
@@ -6475,7 +6478,8 @@ class SubstTemplateTypeParmPackType : public Type, public llvm::FoldingSetNode {
/// This should match the result of `getReplacedParameter()->getIndex()`.
unsigned getIndex() const { return SubstTemplateTypeParmPackTypeBits.Index; }
- // When true the substitution will be 'Final' (subst node won't be placed).
+ // This substitution will be Final, which means the substitution will be fully
+ // sugared: it doesn't need to be resugared later.
bool getFinal() const;
unsigned getNumArgs() const {
diff --git a/clang/include/clang/AST/TypeProperties.td b/clang/include/clang/AST/TypeProperties.td
index 4d89d8a47396a..93f391e99c5ab 100644
--- a/clang/include/clang/AST/TypeProperties.td
+++ b/clang/include/clang/AST/TypeProperties.td
@@ -827,11 +827,11 @@ let Class = SubstTemplateTypeParmType in {
def : Property<"PackIndex", Optional<UInt32>> {
let Read = [{ node->getPackIndex() }];
}
+ def : Property<"Final", Bool> { let Read = [{ node->getFinal() }]; }
- // The call to getCanonicalType here existed in ASTReader.cpp, too.
def : Creator<[{
return ctx.getSubstTemplateTypeParmType(
- replacementType, associatedDecl, Index, PackIndex);
+ replacementType, associatedDecl, Index, PackIndex, Final);
}]>;
}
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 3879cc7942877..930c1c06d1a76 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -161,7 +161,6 @@ LANGOPT(Coroutines , 1, 0, "C++20 coroutines")
LANGOPT(CoroAlignedAllocation, 1, 0, "prefer Aligned Allocation according to P2014 Option 2")
LANGOPT(DllExportInlines , 1, 1, "dllexported classes dllexport inline methods")
LANGOPT(ExperimentalLibrary, 1, 0, "enable unstable and experimental library features")
-LANGOPT(RetainSubstTemplateTypeParmTypeAstNodes, 1, 0, "retain SubstTemplateTypeParmType nodes in the AST's representation of alias template specializations")
LANGOPT(PointerAuthIntrinsics, 1, 0, "pointer authentication intrinsics")
LANGOPT(PointerAuthCalls , 1, 0, "function pointer authentication")
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index fbd5cf632c350..3f68a55e85d8e 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3506,12 +3506,6 @@ defm application_extension : BoolFOption<"application-extension",
PosFlag<SetTrue, [], [ClangOption, CC1Option],
"Restrict code to those available for App Extensions">,
NegFlag<SetFalse>>;
-defm retain_subst_template_type_parm_type_ast_nodes : BoolFOption<"retain-subst-template-type-parm-type-ast-nodes",
- LangOpts<"RetainSubstTemplateTypeParmTypeAstNodes">, DefaultFalse,
- PosFlag<SetTrue, [], [CC1Option], "Enable">,
- NegFlag<SetFalse, [], [], "Disable">,
- BothFlags<[], [], " retain SubstTemplateTypeParmType nodes in the AST's representation"
- " of alias template specializations">>;
defm sized_deallocation : BoolFOption<"sized-deallocation",
LangOpts<"SizedDeallocation">, Default<cpp14.KeyPath>,
PosFlag<SetTrue, [], [], "Enable C++14 sized global deallocation functions">,
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 4d2ca9ec1a123..0e3c6535808e0 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -5449,10 +5449,10 @@ QualType ASTContext::getHLSLAttributedResourceType(
/// Retrieve a substitution-result type.
QualType ASTContext::getSubstTemplateTypeParmType(
QualType Replacement, Decl *AssociatedDecl, unsigned Index,
- std::optional<unsigned> PackIndex) const {
+ std::optional<unsigned> PackIndex, bool Final) const {
llvm::FoldingSetNodeID ID;
SubstTemplateTypeParmType::Profile(ID, Replacement, AssociatedDecl, Index,
- PackIndex);
+ PackIndex, Final);
void *InsertPos = nullptr;
SubstTemplateTypeParmType *SubstParm =
SubstTemplateTypeParmTypes.FindNodeOrInsertPos(ID, InsertPos);
@@ -5462,7 +5462,7 @@ QualType ASTContext::getSubstTemplateTypeParmType(
!Replacement.isCanonical()),
alignof(SubstTemplateTypeParmType));
SubstParm = new (Mem) SubstTemplateTypeParmType(Replacement, AssociatedDecl,
- Index, PackIndex);
+ Index, PackIndex, Final);
Types.push_back(SubstParm);
SubstTemplateTypeParmTypes.InsertNode(SubstParm, InsertPos);
}
@@ -10130,10 +10130,10 @@ ASTContext::getDependentTemplateName(NestedNameSpecifier *NNS,
TemplateName ASTContext::getSubstTemplateTemplateParm(
TemplateName Replacement, Decl *AssociatedDecl, unsigned Index,
- std::optional<unsigned> PackIndex) const {
+ std::optional<unsigned> PackIndex, bool Final) const {
llvm::FoldingSetNodeID ID;
SubstTemplateTemplateParmStorage::Profile(ID, Replacement, AssociatedDecl,
- Index, PackIndex);
+ Index, PackIndex, Final);
void *insertPos = nullptr;
SubstTemplateTemplateParmStorage *subst
@@ -10141,7 +10141,7 @@ TemplateName ASTContext::getSubstTemplateTemplateParm(
if (!subst) {
subst = new (*this) SubstTemplateTemplateParmStorage(
- Replacement, AssociatedDecl, Index, PackIndex);
+ Replacement, AssociatedDecl, Index, PackIndex, Final);
SubstTemplateTemplateParms.InsertNode(subst, insertPos);
}
@@ -14243,7 +14243,8 @@ static QualType getCommonSugarTypeNode(ASTContext &Ctx, const Type *X,
if (PackIndex != SY->getPackIndex())
return QualType();
return Ctx.getSubstTemplateTypeParmType(Ctx.getQualifiedType(Underlying),
- CD, Index, PackIndex);
+ CD, Index, PackIndex,
+ SX->getFinal() && SY->getFinal());
}
case Type::ObjCTypeParam:
// FIXME: Try to merge these.
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index d397c9285ed76..4e27f9d884a83 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -1631,8 +1631,8 @@ ExpectedType ASTNodeImporter::VisitSubstTemplateTypeParmType(
return ToReplacementTypeOrErr.takeError();
return Importer.getToContext().getSubstTemplateTypeParmType(
- *ToReplacementTypeOrErr, *ReplacedOrErr, T->getIndex(),
- T->getPackIndex());
+ *ToReplacementTypeOrErr, *ReplacedOrErr, T->getIndex(), T->getPackIndex(),
+ T->getFinal());
}
ExpectedType ASTNodeImporter::VisitSubstTemplateTypeParmPackType(
@@ -8945,7 +8945,8 @@ ExpectedStmt ASTNodeImporter::VisitSubstNonTypeTemplateParmExpr(
return new (Importer.getToContext()) SubstNonTypeTemplateParmExpr(
ToType, E->getValueKind(), ToExprLoc, ToReplacement, ToAssociatedDecl,
- E->getIndex(), E->getPackIndex(), E->isReferenceParameter());
+ E->getIndex(), E->getPackIndex(), E->isReferenceParameter(),
+ E->getFinal());
}
ExpectedStmt ASTNodeImporter::VisitTypeTraitExpr(TypeTraitExpr *E) {
@@ -9958,7 +9959,7 @@ Expected<TemplateName> ASTImporter::Import(TemplateName From) {
return ToContext.getSubstTemplateTemplateParm(
*ReplacementOrErr, *AssociatedDeclOrErr, Subst->getIndex(),
- Subst->getPackIndex());
+ Subst->getPackIndex(), Subst->getFinal());
}
case TemplateName::SubstTemplateTemplateParmPack: {
diff --git a/clang/lib/AST/ExprCXX.cpp b/clang/lib/AST/ExprCXX.cpp
index a000e988e6834..a0bc50c449d82 100644
--- a/clang/lib/AST/ExprCXX.cpp
+++ b/clang/lib/AST/ExprCXX.cpp
@@ -1760,10 +1760,12 @@ QualType SubstNonTypeTemplateParmExpr::getParameterType(
SubstNonTypeTemplateParmPackExpr::SubstNonTypeTemplateParmPackExpr(
QualType T, ExprValueKind ValueKind, SourceLocation NameLoc,
- const TemplateArgument &ArgPack, Decl *AssociatedDecl, unsigned Index)
+ const TemplateArgument &ArgPack, Decl *AssociatedDecl, unsigned Index,
+ bool Final)
: Expr(SubstNonTypeTemplateParmPackExprClass, T, ValueKind, OK_Ordinary),
AssociatedDecl(AssociatedDecl), Arguments(ArgPack.pack_begin()),
- NumArguments(ArgPack.pack_size()), Index(Index), NameLoc(NameLoc) {
+ NumArguments(ArgPack.pack_size()), Final(Final), Index(Index),
+ NameLoc(NameLoc) {
assert(AssociatedDecl != nullptr);
setDependence(ExprDependence::TypeValueInstantiation |
ExprDependence::UnexpandedPack);
diff --git a/clang/lib/AST/TemplateName.cpp b/clang/lib/AST/TemplateName.cpp
index 9e0a7dc2b8cdc..109cc3c4771ee 100644
--- a/clang/lib/AST/TemplateName.cpp
+++ b/clang/lib/AST/TemplateName.cpp
@@ -77,16 +77,18 @@ SubstTemplateTemplateParmStorage::getParameter() const {
}
void SubstTemplateTemplateParmStorage::Profile(llvm::FoldingSetNodeID &ID) {
- Profile(ID, Replacement, getAssociatedDecl(), getIndex(), getPackIndex());
+ Profile(ID, Replacement, getAssociatedDecl(), getIndex(), getPackIndex(),
+ getFinal());
}
void SubstTemplateTemplateParmStorage::Profile(
llvm::FoldingSetNodeID &ID, TemplateName Replacement, Decl *AssociatedDecl,
- unsigned Index, std::optional<unsigned> PackIndex) {
+ unsigned In...
[truncated]
|
| @@ -265,14 +265,14 @@ int k9 = f9(V9<double>()); | |||
| // CHECK-ELIDE-TREE: S9< | |||
| // CHECK-ELIDE-TREE: [2 * ...], | |||
| // CHECK-ELIDE-TREE: U9< | |||
| // CHECK-ELIDE-TREE: [(no qualifiers) != const] double>> | |||
There was a problem hiding this comment.
It is not obvious why this changed, can you explain?
There was a problem hiding this comment.
Yeah, with the Subst type node, you get difference when printing if the node is top-level qualified, versus when it is bottom-qualified. So you can get harmless differences in the order const appears, and in this case here differences in the order you see qualifiers as you single step desugar the types.
The tree-differ could be improved to remove these inconsistencies, but given the harmless nature of the effect we don't need to do it now.
mizvekov
force-pushed
the
users/mizvekov/GH101754
branch
from
6b4f7cb to
c4cbf3b
Compare
mizvekov
force-pushed
the
users/mizvekov/subst_final
branch
from
f767185 to
4260717
Compare
mizvekov
force-pushed
the
users/mizvekov/GH101754
branch
from
c4cbf3b to
1b3a8e3
Compare
mizvekov
force-pushed
the
users/mizvekov/subst_final
branch
from
4260717 to
db17b41
Compare
mizvekov
force-pushed
the
users/mizvekov/subst_final
branch
from
db17b41 to
3917bdd
Compare
mizvekov
force-pushed
the
users/mizvekov/GH101754
branch
from
1b3a8e3 to
a2a372d
Compare
mizvekov
force-pushed
the
users/mizvekov/subst_final
branch
from
3917bdd to
7db83f2
Compare
There was a problem hiding this comment.
It isn't clear to me where the actual 'work' is happening here, can you point it out? Most of the patch is smuggling the bool around, but I don't see where the actual check/use of final is.
Also-- Are there other opportunities here with this flag we can take advantage of? Anywhere we would attempt to sugar this that we could escape early? If so, maybe we can get part of the 0.25% back.
| @@ -1302,6 +1302,8 @@ void TextNodeDumper::dumpBareTemplateName(TemplateName TN) { | |||
| OS << " index " << STS->getIndex(); | |||
| if (std::optional<unsigned int> PackIndex = STS->getPackIndex()) | |||
| OS << " pack_index " << *PackIndex; | |||
| if (STS->getFinal()) | |||
| OS << " final"; | |||
There was a problem hiding this comment.
as final is a keyword in C++ I wonder if we can find a better name for this. I certainly wouldn't see this in an AST and know what it means. I wonder if we should find a better name than final throughout this patch. Something like, FullySugared or something? Though open to other/better names.
There was a problem hiding this comment.
I am open for suggestions, but I'd prefer to change this in a follow up, since the term is existing and this would make the patch much larger.
There was a problem hiding this comment.
FullySugared seems fine to me, unless you have a better idea. I would be ok with changing the name in a followup, which wouldn't really require review once we agree on a name.
There was a problem hiding this comment.
FullySugaredseems fine to me, unless you have a better idea. I would be ok with changing the name in a followup, which wouldn't really require review once we agree on a name.
+1
The users are external. My resugarer, which I am in the process of upstreaming, uses this flag in order to skip nodes which we shouldn't attempt to resugar. Otherwise, not adding the nodes in the first place was better performing, but the folks who are developing the nullability attribute are asking for it.
Since we don't really use this within upstream clang itself, I don't think there are really any opportunities. |
Ah, got it! I missed that. Thanks! |
There was a problem hiding this comment.
Other than the name, I'm Ok with this. Followup patch to change the name to something more descriptive is very much wanted, but review of that should be trivial once we agree on a name. I am ok with fully-sugared/FullySugared, but open to other suggestions/not particularly attached to that (basically, final being a keyword, then appearing in odd places is going to be a big readability problem).
|
Running some experiments now and will report back soon, thanks! |
mizvekov
force-pushed
the
users/mizvekov/GH101754
branch
from
a2a372d to
6ba9bbe
Compare
Got it, yes not too long, just wanted to make the merges a little smoother. Thanks! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/2721 Here is the relevant piece of the build log for the reference |
…_type_ast_nodes This is a folow-up to #132748, where we deferred the flag removal in order to ease transition for external users.
|
Looks like this change broke the MSAN bot: https://lab.llvm.org/buildbot/#/builders/169/builds/10068 @thurstond FYI |
|
@thurstond Well can you provide a more recent buildbot failure link where #130182 is not on the list of possible culpirits? Because the link @fmayer provided for sure included it. |
@mizvekov I localized the MSan error to this patch by re-running the buildbot script with this patch and immediately prior, in both cases also reverting #130182 to remove noise: Repro: This repro'ed the MSan failures: The MSan failures are not present when using the immediately prior patch: Please let me know if you encounter any issues reproducing the error. |
|
If it is not quick and easy to fix forward, please consider reverting in the meantime, so that the buildbots can be green and provide useful feedback to contributors. Thanks! |
This fixes a couple of mistakes introduced when merging #132748 Fixes msan failure reported here: #132748 (comment)
|
This will be fixed here: #134560 |
Thank you! |
This fixes a couple of mistakes introduced when merging #132748 Fixes msan failure reported here: #132748 (comment)
…(#134560) This fixes a couple of mistakes introduced when merging llvm/llvm-project#132748 Fixes msan failure reported here: llvm/llvm-project#132748 (comment)
|
This causes another mangling issue similar to this. Here's the repro:
The subexpression |
|
With this commit, I get: Before this, the same symbol was: |
|
Ops, you are right, looking into it. |
This fixes a regression introduced here: #132748 which was originally reported here: #132748 (comment) Some time in the clang-20 time frame, we had removed subst* nodes produced from template type alias instantiation. This ended up accidentally fixing an issue where the result from an alias template substitution is mistaken for the susbtitution of an outer non-alias template, incorrectly applying the unresolved-type production to it. This fixes it by ignoring subst* nodes produced from type aliases. Though builtin templates currently don't place subst* nodes, if we ever decide to place them, this fix should cover them as well. No release notes since this regression was never released.
This fixes a regression introduced here: #132748 which was originally reported here: #132748 (comment) Some time in the clang-20 time frame, we had removed subst* nodes produced from template type alias instantiation. This ended up accidentally fixing an issue where the result from an alias template substitution is mistaken for the susbtitution of an outer non-alias template, incorrectly applying the unresolved-type production to it. This fixes it by ignoring subst* nodes produced from type aliases. Though builtin templates currently don't place subst* nodes, if we ever decide to place them, this fix should cover them as well. No release notes since this regression was never released.
…ity code. llvm/llvm-project#132748 makes this flag a no-op PiperOrigin-RevId: 747946560 Change-Id: Id2c92df6b2eb2c62dcdf8bb59e96b8e3cb118723
…-type-ast-nodes (llvm#134177) This is a follow-up to llvm#132748, where we deferred the flag removal in order to ease transition for external users. The plan is to merge this in the nearish future, in two weeks or so is my best guess.
This patch re-adds Subst* nodes for 'Final' substitutions, adding a bit to these nodes to differentiate them from non-Final ones, which for example helps track forward progress in resugaring.
This was originally changed when we added support for sugared substitutions in the template instantiator (326feaa), and later applied to NTTP and default argument substitution (ab11408) and type alias templates (7f78f99).
This kind of instantiation is favored for entities which we don't track specializations, like the aforementioned cases because:
While the node could have been still useful, its removal helped offset the extra costs of the sugared instantiation.
Otherwise, adding them would also necessitate tracking which nodes don't need to be resugared later, which this patch does.
Adding the node back does add a little bit of performance penalty on itself:
0.25% on stage2-clang.
And this will make resugaring a little bit more expensive later when that's upstreamed, since there are more AST nodes to walk, and more AST nodes to produce.
Before:
After:
Which on the current implementation and applications of resugaring transform, is an extra 0.24% on stage2-clang.
Some upstream users reported desirability of keeping these Subst* nodes in all cases, and we had added a special frontend flag to control its removal.
The flag itself will remain for the time being, in order to ease with the transition.
It will be removed in a follow-up.