[Clang][NFC] Remove TemplateArgumentList::OnStack #79760

sdkrystian · 2024-01-28T16:52:56Z

This removes on-stack TemplateArgumentList's. They were primary used to pass an ArrayRef<TemplateArgument> to Sema::getTemplateInstantiationArgs, which had a const TemplateArgumentList* parameter for the innermost template argument list. Changing this parameter to an std::optional<ArrayRef<TemplateArgument>> eliminates the need for on-stack TemplateArgumentList's, which in turn eliminates the need for TemplateArgumentList to store a pointer to its template argument storage (which is redundant in almost all cases, as it is an AST allocated type).

llvmbot · 2024-01-28T16:53:23Z

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

This removes on-stack TemplateArgumentList's. They were primary used to pass an ArrayRef<TemplateArgument> to Sema::getTemplateInstantiationArgs, which had a const TemplateArgumentList* parameter for the innermost template argument list. Changing this parameter to an std::optional<ArrayRef<TemplateArgument>> eliminates the need for on-stack TemplateArgumentList's, which in turn eliminates the need for TemplateArgumentList to store a pointer to its template argument storage (which is redundant in almost all cases, as it is an AST allocated type).

Patch is 27.64 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/79760.diff

9 Files Affected:

(modified) clang/include/clang/AST/DeclTemplate.h (+3-23)
(modified) clang/include/clang/Sema/Sema.h (+4-4)
(modified) clang/lib/AST/DeclTemplate.cpp (+1-2)
(modified) clang/lib/Sema/SemaConcept.cpp (+10-10)
(modified) clang/lib/Sema/SemaExprCXX.cpp (+1-3)
(modified) clang/lib/Sema/SemaTemplate.cpp (+8-12)
(modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+29-41)
(modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+9-6)
(modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+15-12)

diff --git a/clang/include/clang/AST/DeclTemplate.h b/clang/include/clang/AST/DeclTemplate.h
index 832ad2de6b08a82..baf71145d99dc6a 100644
--- a/clang/include/clang/AST/DeclTemplate.h
+++ b/clang/include/clang/AST/DeclTemplate.h
@@ -241,9 +241,6 @@ class FixedSizeTemplateParameterListStorage
 /// A template argument list.
 class TemplateArgumentList final
     : private llvm::TrailingObjects<TemplateArgumentList, TemplateArgument> {
-  /// The template argument list.
-  const TemplateArgument *Arguments;
-
   /// The number of template arguments in this template
   /// argument list.
   unsigned NumArguments;
@@ -258,30 +255,11 @@ class TemplateArgumentList final
   TemplateArgumentList(const TemplateArgumentList &) = delete;
   TemplateArgumentList &operator=(const TemplateArgumentList &) = delete;
 
-  /// Type used to indicate that the template argument list itself is a
-  /// stack object. It does not own its template arguments.
-  enum OnStackType { OnStack };
-
   /// Create a new template argument list that copies the given set of
   /// template arguments.
   static TemplateArgumentList *CreateCopy(ASTContext &Context,
                                           ArrayRef<TemplateArgument> Args);
 
-  /// Construct a new, temporary template argument list on the stack.
-  ///
-  /// The template argument list does not own the template arguments
-  /// provided.
-  explicit TemplateArgumentList(OnStackType, ArrayRef<TemplateArgument> Args)
-      : Arguments(Args.data()), NumArguments(Args.size()) {}
-
-  /// Produces a shallow copy of the given template argument list.
-  ///
-  /// This operation assumes that the input argument list outlives it.
-  /// This takes the list as a pointer to avoid looking like a copy
-  /// constructor, since this really isn't safe to use that way.
-  explicit TemplateArgumentList(const TemplateArgumentList *Other)
-      : Arguments(Other->data()), NumArguments(Other->size()) {}
-
   /// Retrieve the template argument at a given index.
   const TemplateArgument &get(unsigned Idx) const {
     assert(Idx < NumArguments && "Invalid template argument index");
@@ -301,7 +279,9 @@ class TemplateArgumentList final
   unsigned size() const { return NumArguments; }
 
   /// Retrieve a pointer to the template argument list.
-  const TemplateArgument *data() const { return Arguments; }
+  const TemplateArgument *data() const {
+    return getTrailingObjects<TemplateArgument>();
+  }
 };
 
 void *allocateDefaultArgStorageChain(const ASTContext &C);
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index b5946e3f3178ff2..d730c60a406e98f 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -9297,12 +9297,12 @@ class Sema final {
 
   TemplateDeductionResult
   DeduceTemplateArguments(ClassTemplatePartialSpecializationDecl *Partial,
-                          const TemplateArgumentList &TemplateArgs,
+                          ArrayRef<TemplateArgument> TemplateArgs,
                           sema::TemplateDeductionInfo &Info);
 
   TemplateDeductionResult
   DeduceTemplateArguments(VarTemplatePartialSpecializationDecl *Partial,
-                          const TemplateArgumentList &TemplateArgs,
+                          ArrayRef<TemplateArgument> TemplateArgs,
                           sema::TemplateDeductionInfo &Info);
 
   TemplateDeductionResult SubstituteExplicitTemplateArguments(
@@ -9475,7 +9475,7 @@ class Sema final {
 
   MultiLevelTemplateArgumentList getTemplateInstantiationArgs(
       const NamedDecl *D, const DeclContext *DC = nullptr, bool Final = false,
-      const TemplateArgumentList *Innermost = nullptr,
+      std::optional<ArrayRef<TemplateArgument>> Innermost = std::nullopt,
       bool RelativeToPrimary = false, const FunctionDecl *Pattern = nullptr,
       bool ForConstraintInstantiation = false,
       bool SkipForSpecialization = false);
@@ -10467,7 +10467,7 @@ class Sema final {
                                      bool AtEndOfTU = false);
   VarTemplateSpecializationDecl *BuildVarTemplateInstantiation(
       VarTemplateDecl *VarTemplate, VarDecl *FromVar,
-      const TemplateArgumentList &TemplateArgList,
+      const TemplateArgumentList *PartialSpecArgs,
       const TemplateArgumentListInfo &TemplateArgsInfo,
       SmallVectorImpl<TemplateArgument> &Converted,
       SourceLocation PointOfInstantiation,
diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp
index 7d7556e670f951a..f38fe9994fde71c 100644
--- a/clang/lib/AST/DeclTemplate.cpp
+++ b/clang/lib/AST/DeclTemplate.cpp
@@ -871,8 +871,7 @@ void TemplateTemplateParmDecl::setDefaultArgument(
 // TemplateArgumentList Implementation
 //===----------------------------------------------------------------------===//
 TemplateArgumentList::TemplateArgumentList(ArrayRef<TemplateArgument> Args)
-    : Arguments(getTrailingObjects<TemplateArgument>()),
-      NumArguments(Args.size()) {
+    : NumArguments(Args.size()) {
   std::uninitialized_copy(Args.begin(), Args.end(),
                           getTrailingObjects<TemplateArgument>());
 }
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 88fc846c89e42c8..c17149783658713 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -661,11 +661,12 @@ Sema::SetupConstraintCheckingTemplateArgumentsAndScope(
   // Collect the list of template arguments relative to the 'primary' template.
   // We need the entire list, since the constraint is completely uninstantiated
   // at this point.
-  MLTAL = getTemplateInstantiationArgs(FD, FD->getLexicalDeclContext(),
-                                       /*Final=*/false, /*Innermost=*/nullptr,
-                                       /*RelativeToPrimary=*/true,
-                                       /*Pattern=*/nullptr,
-                                       /*ForConstraintInstantiation=*/true);
+  MLTAL =
+      getTemplateInstantiationArgs(FD, FD->getLexicalDeclContext(),
+                                   /*Final=*/false, /*Innermost=*/std::nullopt,
+                                   /*RelativeToPrimary=*/true,
+                                   /*Pattern=*/nullptr,
+                                   /*ForConstraintInstantiation=*/true);
   if (SetupConstraintScope(FD, TemplateArgs, MLTAL, Scope))
     return std::nullopt;
 
@@ -740,7 +741,8 @@ static unsigned
 CalculateTemplateDepthForConstraints(Sema &S, const NamedDecl *ND,
                                      bool SkipForSpecialization = false) {
   MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
-      ND, ND->getLexicalDeclContext(), /*Final=*/false, /*Innermost=*/nullptr,
+      ND, ND->getLexicalDeclContext(), /*Final=*/false,
+      /*Innermost=*/std::nullopt,
       /*RelativeToPrimary=*/true,
       /*Pattern=*/nullptr,
       /*ForConstraintInstantiation=*/true, SkipForSpecialization);
@@ -780,7 +782,7 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
     const Expr *ConstrExpr) {
   MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
       DeclInfo.getDecl(), DeclInfo.getLexicalDeclContext(), /*Final=*/false,
-      /*Innermost=*/nullptr,
+      /*Innermost=*/std::nullopt,
       /*RelativeToPrimary=*/true,
       /*Pattern=*/nullptr, /*ForConstraintInstantiation=*/true,
       /*SkipForSpecialization*/ false);
@@ -1258,11 +1260,9 @@ substituteParameterMappings(Sema &S, NormalizedConstraint &N,
 
 static bool substituteParameterMappings(Sema &S, NormalizedConstraint &N,
                                         const ConceptSpecializationExpr *CSE) {
-  TemplateArgumentList TAL{TemplateArgumentList::OnStack,
-                           CSE->getTemplateArguments()};
   MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
       CSE->getNamedConcept(), CSE->getNamedConcept()->getLexicalDeclContext(),
-      /*Final=*/false, &TAL,
+      /*Final=*/false, CSE->getTemplateArguments(),
       /*RelativeToPrimary=*/true,
       /*Pattern=*/nullptr,
       /*ForConstraintInstantiation=*/true);
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 51c61886739bb91..39f67b2b743cb3f 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -9107,9 +9107,7 @@ Sema::BuildExprRequirement(
 
     auto *Param = cast<TemplateTypeParmDecl>(TPL->getParam(0));
 
-    TemplateArgumentList TAL(TemplateArgumentList::OnStack, Args);
-    MultiLevelTemplateArgumentList MLTAL(Param, TAL.asArray(),
-                                         /*Final=*/false);
+    MultiLevelTemplateArgumentList MLTAL(Param, Args, /*Final=*/false);
     MLTAL.addOuterRetainedLevels(TPL->getDepth());
     const TypeConstraint *TC = Param->getTypeConstraint();
     assert(TC && "Type Constraint cannot be null here");
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 3f33ecb89502eaf..255de435f648026 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -4843,9 +4843,7 @@ Sema::CheckVarTemplateId(VarTemplateDecl *Template, SourceLocation TemplateLoc,
   // the set of specializations, based on the closest partial specialization
   // that it represents. That is,
   VarDecl *InstantiationPattern = Template->getTemplatedDecl();
-  TemplateArgumentList TemplateArgList(TemplateArgumentList::OnStack,
-                                       CanonicalConverted);
-  TemplateArgumentList *InstantiationArgs = &TemplateArgList;
+  const TemplateArgumentList *PartialSpecArgs = nullptr;
   bool AmbiguousPartialSpec = false;
   typedef PartialSpecMatchResult MatchResult;
   SmallVector<MatchResult, 4> Matched;
@@ -4866,7 +4864,7 @@ Sema::CheckVarTemplateId(VarTemplateDecl *Template, SourceLocation TemplateLoc,
     TemplateDeductionInfo Info(FailedCandidates.getLocation());
 
     if (TemplateDeductionResult Result =
-            DeduceTemplateArguments(Partial, TemplateArgList, Info)) {
+            DeduceTemplateArguments(Partial, CanonicalConverted, Info)) {
       // Store the failed-deduction information for use in diagnostics, later.
       // TODO: Actually use the failed-deduction info?
       FailedCandidates.addCandidate().set(
@@ -4919,7 +4917,7 @@ Sema::CheckVarTemplateId(VarTemplateDecl *Template, SourceLocation TemplateLoc,
 
     // Instantiate using the best variable template partial specialization.
     InstantiationPattern = Best->Partial;
-    InstantiationArgs = Best->Args;
+    PartialSpecArgs = Best->Args;
   } else {
     //   -- If no match is found, the instantiation is generated
     //      from the primary template.
@@ -4931,7 +4929,7 @@ Sema::CheckVarTemplateId(VarTemplateDecl *Template, SourceLocation TemplateLoc,
   // in DoMarkVarDeclReferenced().
   // FIXME: LateAttrs et al.?
   VarTemplateSpecializationDecl *Decl = BuildVarTemplateInstantiation(
-      Template, InstantiationPattern, *InstantiationArgs, TemplateArgs,
+      Template, InstantiationPattern, PartialSpecArgs, TemplateArgs,
       CanonicalConverted, TemplateNameLoc /*, LateAttrs, StartingScope*/);
   if (!Decl)
     return true;
@@ -4952,7 +4950,7 @@ Sema::CheckVarTemplateId(VarTemplateDecl *Template, SourceLocation TemplateLoc,
 
   if (VarTemplatePartialSpecializationDecl *D =
           dyn_cast<VarTemplatePartialSpecializationDecl>(InstantiationPattern))
-    Decl->setInstantiationOf(D, InstantiationArgs);
+    Decl->setInstantiationOf(D, PartialSpecArgs);
 
   checkSpecializationReachability(TemplateNameLoc, Decl);
 
@@ -6257,8 +6255,6 @@ bool Sema::CheckTemplateArgumentList(
     TemplateArgs = std::move(NewArgs);
 
   if (!PartialTemplateArgs) {
-    TemplateArgumentList StackTemplateArgs(TemplateArgumentList::OnStack,
-                                           CanonicalConverted);
     // Setup the context/ThisScope for the case where we are needing to
     // re-instantiate constraints outside of normal instantiation.
     DeclContext *NewContext = Template->getDeclContext();
@@ -6278,7 +6274,7 @@ bool Sema::CheckTemplateArgumentList(
     CXXThisScopeRAII(*this, RD, ThisQuals, RD != nullptr);
 
     MultiLevelTemplateArgumentList MLTAL = getTemplateInstantiationArgs(
-        Template, NewContext, /*Final=*/false, &StackTemplateArgs,
+        Template, NewContext, /*Final=*/false, CanonicalConverted,
         /*RelativeToPrimary=*/true,
         /*Pattern=*/nullptr,
         /*ForConceptInstantiation=*/true);
@@ -9778,8 +9774,8 @@ bool Sema::CheckFunctionTemplateSpecialization(
   // specialization, with the template arguments from the previous
   // specialization.
   // Take copies of (semantic and syntactic) template argument lists.
-  const TemplateArgumentList* TemplArgs = new (Context)
-    TemplateArgumentList(Specialization->getTemplateSpecializationArgs());
+  const TemplateArgumentList *TemplArgs = TemplateArgumentList::CreateCopy(
+      Context, Specialization->getTemplateSpecializationArgs()->asArray());
   FD->setFunctionTemplateSpecialization(
       Specialization->getPrimaryTemplate(), TemplArgs, /*InsertPos=*/nullptr,
       SpecInfo->getTemplateSpecializationKind(),
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index 25e58f7bdd953d1..720712ed4814252 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -2513,17 +2513,6 @@ DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
   return Sema::TDK_Success;
 }
 
-static Sema::TemplateDeductionResult
-DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
-                        const TemplateArgumentList &ParamList,
-                        const TemplateArgumentList &ArgList,
-                        TemplateDeductionInfo &Info,
-                        SmallVectorImpl<DeducedTemplateArgument> &Deduced) {
-  return DeduceTemplateArguments(S, TemplateParams, ParamList.asArray(),
-                                 ArgList.asArray(), Info, Deduced,
-                                 /*NumberOfArgumentsMustMatch=*/false);
-}
-
 /// Determine whether two template arguments are the same.
 static bool isSameTemplateArg(ASTContext &Context,
                               TemplateArgument X,
@@ -2944,13 +2933,14 @@ CheckDeducedArgumentConstraints(Sema &S, TemplateDeclT *Template,
   llvm::SmallVector<const Expr *, 3> AssociatedConstraints;
   Template->getAssociatedConstraints(AssociatedConstraints);
 
-  bool NeedsReplacement = DeducedArgsNeedReplacement(Template);
-  TemplateArgumentList DeducedTAL{TemplateArgumentList::OnStack,
-                                  CanonicalDeducedArgs};
+  std::optional<ArrayRef<TemplateArgument>> Innermost;
+  // If we don't need to replace the deduced template arguments,
+  // we can add them immediately as the inner-most argument list.
+  if (!DeducedArgsNeedReplacement(Template))
+    Innermost = CanonicalDeducedArgs;
 
   MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
-      Template, Template->getDeclContext(), /*Final=*/false,
-      /*InnerMost=*/NeedsReplacement ? nullptr : &DeducedTAL,
+      Template, Template->getDeclContext(), /*Final=*/false, Innermost,
       /*RelativeToPrimary=*/true, /*Pattern=*/
       nullptr, /*ForConstraintInstantiation=*/true);
 
@@ -2958,7 +2948,7 @@ CheckDeducedArgumentConstraints(Sema &S, TemplateDeclT *Template,
   // template args when this is a variable template partial specialization and
   // not class-scope explicit specialization, so replace with Deduced Args
   // instead of adding to inner-most.
-  if (NeedsReplacement)
+  if (!Innermost)
     MLTAL.replaceInnermostTemplateArguments(Template, CanonicalDeducedArgs);
 
   if (S.CheckConstraintSatisfaction(Template, AssociatedConstraints, MLTAL,
@@ -2979,7 +2969,7 @@ static std::enable_if_t<IsPartialSpecialization<T>::value,
                         Sema::TemplateDeductionResult>
 FinishTemplateArgumentDeduction(
     Sema &S, T *Partial, bool IsPartialOrdering,
-    const TemplateArgumentList &TemplateArgs,
+    ArrayRef<TemplateArgument> TemplateArgs,
     SmallVectorImpl<DeducedTemplateArgument> &Deduced,
     TemplateDeductionInfo &Info) {
   // Unevaluated SFINAE context.
@@ -3072,7 +3062,7 @@ FinishTemplateArgumentDeduction(
 // FIXME: Factor out duplication with partial specialization version above.
 static Sema::TemplateDeductionResult FinishTemplateArgumentDeduction(
     Sema &S, TemplateDecl *Template, bool PartialOrdering,
-    const TemplateArgumentList &TemplateArgs,
+    ArrayRef<TemplateArgument> TemplateArgs,
     SmallVectorImpl<DeducedTemplateArgument> &Deduced,
     TemplateDeductionInfo &Info) {
   // Unevaluated SFINAE context.
@@ -3121,7 +3111,7 @@ static Sema::TemplateDeductionResult FinishTemplateArgumentDeduction(
 /// partial specialization per C++ [temp.class.spec.match].
 Sema::TemplateDeductionResult
 Sema::DeduceTemplateArguments(ClassTemplatePartialSpecializationDecl *Partial,
-                              const TemplateArgumentList &TemplateArgs,
+                              ArrayRef<TemplateArgument> TemplateArgs,
                               TemplateDeductionInfo &Info) {
   if (Partial->isInvalidDecl())
     return TDK_Invalid;
@@ -3143,11 +3133,10 @@ Sema::DeduceTemplateArguments(ClassTemplatePartialSpecializationDecl *Partial,
 
   SmallVector<DeducedTemplateArgument, 4> Deduced;
   Deduced.resize(Partial->getTemplateParameters()->size());
-  if (TemplateDeductionResult Result
-        = ::DeduceTemplateArguments(*this,
-                                    Partial->getTemplateParameters(),
-                                    Partial->getTemplateArgs(),
-                                    TemplateArgs, Info, Deduced))
+  if (TemplateDeductionResult Result = ::DeduceTemplateArguments(
+          *this, Partial->getTemplateParameters(),
+          Partial->getTemplateArgs().asArray(), TemplateArgs, Info, Deduced,
+          /*NumberOfArgumentsMustMatch=*/false))
     return Result;
 
   SmallVector<TemplateArgument, 4> DeducedArgs(Deduced.begin(), Deduced.end());
@@ -3173,7 +3162,7 @@ Sema::DeduceTemplateArguments(ClassTemplatePartialSpecializationDecl *Partial,
 /// partial specialization per C++ [temp.class.spec.match].
 Sema::TemplateDeductionResult
 Sema::DeduceTemplateArguments(VarTemplatePartialSpecializationDecl *Partial,
-                              const TemplateArgumentList &TemplateArgs,
+                              ArrayRef<TemplateArgument> TemplateArgs,
                               TemplateDeductionInfo &Info) {
   if (Partial->isInvalidDecl())
     return TDK_Invalid;
@@ -3196,8 +3185,9 @@ Sema::DeduceTemplateArguments(VarTemplatePartialSpecializationDecl *Partial,
   SmallVector<DeducedTemplateArgument, 4> Deduced;
   Deduced.resize(Partial->getTemplateParameters()->size());
   if (TemplateDeductionResult Result = ::DeduceTemplateArguments(
-          *this, Partial->getTemplateParameters(), Partial->getTemplateArgs(),
-          TemplateArgs, Info, Deduced))
+          *this, Partial->getTemplateParameters(),
+          Partial->getTemplateArgs().asArray(), TemplateArgs, Info, Deduced,
+          /*NumberOfArgumentsMustMatch=*/false))
     return Result;
 
   SmallVector<TemplateArgument, 4> DeducedArgs(Deduced.begin(), Deduced.end());
@@ -3426,15 +3416,15 @@ Sema::TemplateDeductionResult Sema::SubstituteExplicitTemplateArguments(
     // specification.
     SmallVector<QualType, 4> ExceptionStorage;
     if (getLangOpts().CPlusPlus17 &&
-        SubstExceptionSpec(Function->getLocation(), EPI.ExceptionSpec,
-                           ExceptionStorage,
-                           getTemplateInstantiationArgs(
-                               FunctionTemplate, nullptr, /*Final=*/true,
-                               /*Innermost=*/SugaredExplicitArgumentList,
-                               /*RelativeToPrimary=*/false,
-                               /*Pattern=*/nullptr,
-                               /*ForConstraintInstantiation=*/false,
-                               /*SkipForSpecialization=*/true)))
+        SubstExceptionSpec(
+            Function->getLocation(), EPI.ExceptionSpec, ExceptionStorage,
+            getTemplateInstantiationArgs(
+                FunctionTemplate, nullptr, /*Final=*/true,
+                /*Innermost=*/SugaredExplicitArgumentList->asArray(),
+                /*RelativeToPrimary=*/false,
+ ...
[truncated]

erichkeane

So I like this! I think it is a good change. I WOULD like to see a release note. I know this isn't particularly 'user visible', but I think, paired with some sort of benchmark, that this would be useful to brag about.

We often run out of stack space while instantiating templates that are particularly large/deep despite our best efforts, and I have a feeling that both saving space in the TAL plus not storing anythign on the stack would improve that.

@cor3ntin : FYI, this would be interesting to be aware of. Comments on how a release note could go are also appreciated.

cor3ntin · 2024-01-29T20:42:18Z

clang/include/clang/AST/DeclTemplate.h

  unsigned size() const { return NumArguments; }

  /// Retrieve a pointer to the template argument list.
-  const TemplateArgument *data() const { return Arguments; }
+  const TemplateArgument *data() const {
+    return getTrailingObjects<TemplateArgument>();
+  }


I wonder if we could get rid of data and size entirely in the public interface

We definitely could, but perhaps that is outside the scope of this PR? That being said, It would be good to unify the interfaces of TemplateArgumentList, TemplateArgumentListInfo, and ASTTemplateArgumentListInfo by perhaps giving them all the following members:

begin() and end()

size() and empty()

operator[]

operator ArrayRef<TemplateArgument>()/operator ArrayRef<TemplateArgumentInfo>()

sdkrystian · 2024-02-01T12:57:08Z

@erichkeane I added the following release note:

Removed support for constructing on-stack TemplateArgumentLists. Interfaces should instead use ArrayRef<TemplateArgument> to pass template arguments.

I'm not entirely sure how to benchmark these changes. Perhaps I could determine the difference in AST memory usage compiling something like Boost or LLVM?

erichkeane · 2024-02-01T15:01:12Z

@erichkeane I added the following release note:

Removed support for constructing on-stack TemplateArgumentLists. Interfaces should instead use ArrayRef<TemplateArgument> to pass template arguments.

I'm not entirely sure how to benchmark these changes. Perhaps I could determine the difference in AST memory usage compiling something like Boost or LLVM?

Boost is a great one I think, a memory profile of that would be great. Even a 'rough' number (this decreased the memory pressure of compiling boost by 1.1%!) would be something good to 'show off'.

I think that is the good 'first half' of a release note, the second half giving justification for it (again, this both simplifies the code and decreases memory pressure by X% sorta thing)..

sdkrystian · 2024-02-01T15:37:18Z

@erichkeane I ended up writing a python script that compiles the TUs in clang/lib with -print-stats and then sums up the total bytes allocated by ASTContext. Ended up with a 0.4% decrease in memory usage.

This does however rely on compile_commands.json, and boost does not have a great way to generate this for every subproject... Do you think the results from compiling clang are sufficient?

erichkeane · 2024-02-01T15:41:29Z

@erichkeane I ended up writing a python script that compiles the TUs in clang/lib with -print-stats and then sums up the total bytes allocated by ASTContext. Ended up with a 0.4% decrease in memory usage.

This does however rely on compile_commands.json, and boost does not have a great way to generate this for every subproject... Do you think the results from compiling clang are sufficient?

Yeah, I think that is sufficient, find a way to write that as prose in the release note, and i'm happy with it.

sdkrystian · 2024-02-01T15:59:38Z

@erichkeane How does this sound?

Removed support for constructing on-stack TemplateArgumentLists; interfaces should instead use ArrayRef<TemplateArgument> to pass template arguments. This reduces AST memory usage by 0.4% when compiling clang.

erichkeane · 2024-02-01T16:02:12Z

@erichkeane How does this sound?

Removed support for constructing on-stack TemplateArgumentLists; interfaces should instead use ArrayRef<TemplateArgument> to pass template arguments. Transitioning internal uses to ArrayRef<TemplateArgument> reduces AST memory usage by 0.4% when compiling clang, and is expected to show similar improvements on other workloads.

See edited quote^

This patch removes on-stack `TemplateArgumentList`'s. They were primary used to pass an `ArrayRef<TemplateArgument>` to `Sema::getTemplateInstantiationArgs`, which had a `const TemplateArgumentList*` parameter for the innermost template argument list. Changing this parameter to an `std::optional<ArrayRef<TemplateArgument>>` eliminates the need for on-stack `TemplateArgumentList`'s, which in turn eliminates the need for `TemplateArgumentList` to store a pointer to its template argument storage (which is redundant in almost all cases, as it is an AST allocated type).

sdkrystian requested a review from erichkeane January 28, 2024 16:52

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 28, 2024

erichkeane reviewed Jan 29, 2024

View reviewed changes

cor3ntin reviewed Jan 29, 2024

View reviewed changes

sdkrystian force-pushed the remove-on-stack branch 2 times, most recently from 1f6d9f3 to 87b8ab6 Compare February 1, 2024 12:38

sdkrystian force-pushed the remove-on-stack branch from 87b8ab6 to 2e3e1a1 Compare February 1, 2024 15:57

[Clang][NFC] Remove TemplateArgumentList::OnStack

2ea817f

sdkrystian force-pushed the remove-on-stack branch from 2e3e1a1 to 2ea817f Compare February 1, 2024 16:13

erichkeane approved these changes Feb 1, 2024

View reviewed changes

sdkrystian merged commit 4739a97 into llvm:main Feb 1, 2024
4 of 5 checks passed

sdkrystian deleted the remove-on-stack branch February 26, 2024 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang][NFC] Remove TemplateArgumentList::OnStack #79760

[Clang][NFC] Remove TemplateArgumentList::OnStack #79760

sdkrystian commented Jan 28, 2024

llvmbot commented Jan 28, 2024

erichkeane left a comment

cor3ntin Jan 29, 2024 •

edited

sdkrystian Jan 30, 2024 •

edited

sdkrystian commented Feb 1, 2024

erichkeane commented Feb 1, 2024

sdkrystian commented Feb 1, 2024

erichkeane commented Feb 1, 2024

sdkrystian commented Feb 1, 2024

erichkeane commented Feb 1, 2024

[Clang][NFC] Remove TemplateArgumentList::OnStack #79760

[Clang][NFC] Remove TemplateArgumentList::OnStack #79760

Conversation

sdkrystian commented Jan 28, 2024

llvmbot commented Jan 28, 2024

erichkeane left a comment

Choose a reason for hiding this comment

cor3ntin Jan 29, 2024 • edited

Choose a reason for hiding this comment

sdkrystian Jan 30, 2024 • edited

Choose a reason for hiding this comment

sdkrystian commented Feb 1, 2024

erichkeane commented Feb 1, 2024

sdkrystian commented Feb 1, 2024

erichkeane commented Feb 1, 2024

sdkrystian commented Feb 1, 2024

erichkeane commented Feb 1, 2024

cor3ntin Jan 29, 2024 •

edited

sdkrystian Jan 30, 2024 •

edited