Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one. #83108

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ChuanqiXu9
Copy link
Member

This from https://reviews.llvm.org/D41416. And we plan to introduce on disk hash table based on this. See #76774.

Following off are cited from https://reviews.llvm.org/D41416:

Currently, we load all lazy template specializations when we search whether there is a suitable template specialization for a template. This is especially suboptimal with modules. If module B specializes a template from module A the ASTReader would only read the specialization DeclID. This is observed to be especially pathological when module B is stl. However, the template instantiator (almost immediately after) will call findSpecialization. In turn, findSpecialization will load all lazy specializations to give an answer.

This patch teaches findSpecialization to work with lazy specializations without having to deserialize their full content. It provides along with the DeclID an cross-TU stable ODRHash of the template arguments which is enough to decide if we have already specialization and which are the exact ones (we load all decls with the same hash to avoid potential collisions) to deserialize.

While we make finding a template specialization more memory-efficient we are far from being done. There are still a few places which trigger eager deserialization of template specializations: the places where we require completion of the redeclaration chain.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Feb 27, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 27, 2024

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Chuanqi Xu (ChuanqiXu9)

Changes

This from https://reviews.llvm.org/D41416. And we plan to introduce on disk hash table based on this. See #76774.

Following off are cited from https://reviews.llvm.org/D41416:

Currently, we load all lazy template specializations when we search whether there is a suitable template specialization for a template. This is especially suboptimal with modules. If module B specializes a template from module A the ASTReader would only read the specialization DeclID. This is observed to be especially pathological when module B is stl. However, the template instantiator (almost immediately after) will call findSpecialization. In turn, findSpecialization will load all lazy specializations to give an answer.

This patch teaches findSpecialization to work with lazy specializations without having to deserialize their full content. It provides along with the DeclID an cross-TU stable ODRHash of the template arguments which is enough to decide if we have already specialization and which are the exact ones (we load all decls with the same hash to avoid potential collisions) to deserialize.

While we make finding a template specialization more memory-efficient we are far from being done. There are still a few places which trigger eager deserialization of template specializations: the places where we require completion of the redeclaration chain.


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

7 Files Affected:

  • (modified) clang/include/clang/AST/DeclTemplate.h (+32-4)
  • (modified) clang/lib/AST/DeclTemplate.cpp (+74-22)
  • (modified) clang/lib/AST/ODRHash.cpp (+15)
  • (modified) clang/lib/Serialization/ASTReader.cpp (+17-8)
  • (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+23-13)
  • (modified) clang/lib/Serialization/ASTWriter.cpp (+19-2)
  • (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+62-12)
diff --git a/clang/include/clang/AST/DeclTemplate.h b/clang/include/clang/AST/DeclTemplate.h
index e3b6a7efb1127a..4ed9b58d4ff609 100644
--- a/clang/include/clang/AST/DeclTemplate.h
+++ b/clang/include/clang/AST/DeclTemplate.h
@@ -256,6 +256,9 @@ class TemplateArgumentList final
   TemplateArgumentList(const TemplateArgumentList &) = delete;
   TemplateArgumentList &operator=(const TemplateArgumentList &) = delete;
 
+  /// Create hash for the given arguments.
+  static unsigned ComputeODRHash(ArrayRef<TemplateArgument> Args);
+
   /// Create a new template argument list that copies the given set of
   /// template arguments.
   static TemplateArgumentList *CreateCopy(ASTContext &Context,
@@ -730,6 +733,26 @@ class RedeclarableTemplateDecl : public TemplateDecl,
   }
 
   void anchor() override;
+  struct LazySpecializationInfo {
+    uint32_t DeclID = ~0U;
+    unsigned ODRHash = ~0U;
+    bool IsPartial = false;
+    LazySpecializationInfo(uint32_t ID, unsigned Hash = ~0U,
+                           bool Partial = false)
+      : DeclID(ID), ODRHash(Hash), IsPartial(Partial) { }
+    LazySpecializationInfo() { }
+    bool operator<(const LazySpecializationInfo &Other) const {
+      return DeclID < Other.DeclID;
+    }
+    bool operator==(const LazySpecializationInfo &Other) const {
+      assert((DeclID != Other.DeclID || ODRHash == Other.ODRHash) &&
+             "Hashes differ!");
+      assert((DeclID != Other.DeclID || IsPartial == Other.IsPartial) &&
+             "Both must be the same kinds!");
+      return DeclID == Other.DeclID;
+    }
+  };
+
 protected:
   template <typename EntryType> struct SpecEntryTraits {
     using DeclType = EntryType;
@@ -770,7 +793,12 @@ class RedeclarableTemplateDecl : public TemplateDecl,
     return SpecIterator<EntryType>(isEnd ? Specs.end() : Specs.begin());
   }
 
-  void loadLazySpecializationsImpl() const;
+  void loadLazySpecializationsImpl(bool OnlyPartial = false) const;
+
+  void loadLazySpecializationsImpl(llvm::ArrayRef<TemplateArgument> Args,
+                                   TemplateParameterList *TPL = nullptr) const;
+
+  Decl *loadLazySpecializationImpl(LazySpecializationInfo &LazySpecInfo) const;
 
   template <class EntryType, typename ...ProfileArguments>
   typename SpecEntryTraits<EntryType>::DeclType*
@@ -797,7 +825,7 @@ class RedeclarableTemplateDecl : public TemplateDecl,
     ///
     /// The first value in the array is the number of specializations/partial
     /// specializations that follow.
-    uint32_t *LazySpecializations = nullptr;
+    LazySpecializationInfo *LazySpecializations = nullptr;
 
     /// The set of "injected" template arguments used within this
     /// template.
@@ -2268,7 +2296,7 @@ class ClassTemplateDecl : public RedeclarableTemplateDecl {
   friend class TemplateDeclInstantiator;
 
   /// Load any lazily-loaded specializations from the external source.
-  void LoadLazySpecializations() const;
+  void LoadLazySpecializations(bool OnlyPartial = false) const;
 
   /// Get the underlying class declarations of the template.
   CXXRecordDecl *getTemplatedDecl() const {
@@ -3039,7 +3067,7 @@ class VarTemplateDecl : public RedeclarableTemplateDecl {
   friend class ASTDeclWriter;
 
   /// Load any lazily-loaded specializations from the external source.
-  void LoadLazySpecializations() const;
+  void LoadLazySpecializations(bool OnlyPartial = false) const;
 
   /// Get the underlying variable declarations of the template.
   VarDecl *getTemplatedDecl() const {
diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp
index 3c217d6a6a5ae3..1babe39ee2a7e5 100644
--- a/clang/lib/AST/DeclTemplate.cpp
+++ b/clang/lib/AST/DeclTemplate.cpp
@@ -20,6 +20,8 @@
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
+#include "clang/AST/ODRHash.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/LLVM.h"
@@ -331,16 +333,43 @@ RedeclarableTemplateDecl::CommonBase *RedeclarableTemplateDecl::getCommonPtr() c
   return Common;
 }
 
-void RedeclarableTemplateDecl::loadLazySpecializationsImpl() const {
+void RedeclarableTemplateDecl::loadLazySpecializationsImpl(
+                                             bool OnlyPartial/*=false*/) const {
   // Grab the most recent declaration to ensure we've loaded any lazy
   // redeclarations of this template.
   CommonBase *CommonBasePtr = getMostRecentDecl()->getCommonPtr();
-  if (CommonBasePtr->LazySpecializations) {
-    ASTContext &Context = getASTContext();
-    uint32_t *Specs = CommonBasePtr->LazySpecializations;
-    CommonBasePtr->LazySpecializations = nullptr;
-    for (uint32_t I = 0, N = *Specs++; I != N; ++I)
-      (void)Context.getExternalSource()->GetExternalDecl(Specs[I]);
+  if (auto *Specs = CommonBasePtr->LazySpecializations) {
+    if (!OnlyPartial)
+      CommonBasePtr->LazySpecializations = nullptr;
+    for (uint32_t I = 0, N = Specs[0].DeclID; I != N; ++I) {
+      // Skip over already loaded specializations.
+      if (!Specs[I+1].ODRHash)
+        continue;
+      if (!OnlyPartial || Specs[I+1].IsPartial)
+        (void)loadLazySpecializationImpl(Specs[I+1]);
+    }
+  }
+}
+
+Decl *RedeclarableTemplateDecl::loadLazySpecializationImpl(
+                                   LazySpecializationInfo &LazySpecInfo) const {
+  uint32_t ID = LazySpecInfo.DeclID;
+  assert(ID && "Loading already loaded specialization!");
+  // Note that we loaded the specialization.
+  LazySpecInfo.DeclID = LazySpecInfo.ODRHash = LazySpecInfo.IsPartial = 0;
+  return getASTContext().getExternalSource()->GetExternalDecl(ID);
+}
+
+void
+RedeclarableTemplateDecl::loadLazySpecializationsImpl(ArrayRef<TemplateArgument>
+                                                      Args,
+                                                      TemplateParameterList *TPL) const {
+  CommonBase *CommonBasePtr = getMostRecentDecl()->getCommonPtr();
+  if (auto *Specs = CommonBasePtr->LazySpecializations) {
+    unsigned Hash = TemplateArgumentList::ComputeODRHash(Args);
+    for (uint32_t I = 0, N = Specs[0].DeclID; I != N; ++I)
+      if (Specs[I+1].ODRHash && Specs[I+1].ODRHash == Hash)
+        (void)loadLazySpecializationImpl(Specs[I+1]);
   }
 }
 
@@ -351,6 +380,8 @@ RedeclarableTemplateDecl::findSpecializationImpl(
     ProfileArguments&&... ProfileArgs) {
   using SETraits = SpecEntryTraits<EntryType>;
 
+  loadLazySpecializationsImpl(std::forward<ProfileArguments>(ProfileArgs)...);
+
   llvm::FoldingSetNodeID ID;
   EntryType::Profile(ID, std::forward<ProfileArguments>(ProfileArgs)...,
                      getASTContext());
@@ -366,10 +397,14 @@ void RedeclarableTemplateDecl::addSpecializationImpl(
 
   if (InsertPos) {
 #ifndef NDEBUG
+    auto Args = SETraits::getTemplateArgs(Entry);
+    // Due to hash collisions, it can happen that we load another template
+    // specialization with the same hash. This is fine, as long as the next
+    // call to findSpecializationImpl does not find a matching Decl for the
+    // template arguments.
+    loadLazySpecializationsImpl(Args);
     void *CorrectInsertPos;
-    assert(!findSpecializationImpl(Specializations,
-                                   CorrectInsertPos,
-                                   SETraits::getTemplateArgs(Entry)) &&
+    assert(!findSpecializationImpl(Specializations, CorrectInsertPos, Args) &&
            InsertPos == CorrectInsertPos &&
            "given incorrect InsertPos for specialization");
 #endif
@@ -443,12 +478,14 @@ FunctionTemplateDecl::getSpecializations() const {
 FunctionDecl *
 FunctionTemplateDecl::findSpecialization(ArrayRef<TemplateArgument> Args,
                                          void *&InsertPos) {
-  return findSpecializationImpl(getSpecializations(), InsertPos, Args);
+  auto *Common = getCommonPtr();
+  return findSpecializationImpl(Common->Specializations, InsertPos, Args);
 }
 
 void FunctionTemplateDecl::addSpecialization(
       FunctionTemplateSpecializationInfo *Info, void *InsertPos) {
-  addSpecializationImpl<FunctionTemplateDecl>(getSpecializations(), Info,
+  auto *Common = getCommonPtr();
+  addSpecializationImpl<FunctionTemplateDecl>(Common->Specializations, Info,
                                               InsertPos);
 }
 
@@ -508,8 +545,9 @@ ClassTemplateDecl *ClassTemplateDecl::CreateDeserialized(ASTContext &C,
                                        DeclarationName(), nullptr, nullptr);
 }
 
-void ClassTemplateDecl::LoadLazySpecializations() const {
-  loadLazySpecializationsImpl();
+void ClassTemplateDecl::LoadLazySpecializations(
+                                             bool OnlyPartial/*=false*/) const {
+  loadLazySpecializationsImpl(OnlyPartial);
 }
 
 llvm::FoldingSetVector<ClassTemplateSpecializationDecl> &
@@ -520,7 +558,7 @@ ClassTemplateDecl::getSpecializations() const {
 
 llvm::FoldingSetVector<ClassTemplatePartialSpecializationDecl> &
 ClassTemplateDecl::getPartialSpecializations() const {
-  LoadLazySpecializations();
+  LoadLazySpecializations(/*PartialOnly = */ true);
   return getCommonPtr()->PartialSpecializations;
 }
 
@@ -534,12 +572,15 @@ ClassTemplateDecl::newCommon(ASTContext &C) const {
 ClassTemplateSpecializationDecl *
 ClassTemplateDecl::findSpecialization(ArrayRef<TemplateArgument> Args,
                                       void *&InsertPos) {
-  return findSpecializationImpl(getSpecializations(), InsertPos, Args);
+  auto *Common = getCommonPtr();
+  return findSpecializationImpl(Common->Specializations, InsertPos, Args);
 }
 
 void ClassTemplateDecl::AddSpecialization(ClassTemplateSpecializationDecl *D,
                                           void *InsertPos) {
-  addSpecializationImpl<ClassTemplateDecl>(getSpecializations(), D, InsertPos);
+  auto *Common = getCommonPtr();
+  addSpecializationImpl<ClassTemplateDecl>(Common->Specializations, D,
+                                           InsertPos);
 }
 
 ClassTemplatePartialSpecializationDecl *
@@ -883,6 +924,14 @@ TemplateArgumentList::CreateCopy(ASTContext &Context,
   return new (Mem) TemplateArgumentList(Args);
 }
 
+unsigned TemplateArgumentList::ComputeODRHash(ArrayRef<TemplateArgument> Args) {
+  ODRHash Hasher;
+  for (TemplateArgument TA : Args)
+    Hasher.AddTemplateArgument(TA);
+
+  return Hasher.CalculateHash();
+}
+
 FunctionTemplateSpecializationInfo *FunctionTemplateSpecializationInfo::Create(
     ASTContext &C, FunctionDecl *FD, FunctionTemplateDecl *Template,
     TemplateSpecializationKind TSK, const TemplateArgumentList *TemplateArgs,
@@ -1225,8 +1274,9 @@ VarTemplateDecl *VarTemplateDecl::CreateDeserialized(ASTContext &C,
                                      DeclarationName(), nullptr, nullptr);
 }
 
-void VarTemplateDecl::LoadLazySpecializations() const {
-  loadLazySpecializationsImpl();
+void VarTemplateDecl::LoadLazySpecializations(
+                                             bool OnlyPartial/*=false*/) const {
+  loadLazySpecializationsImpl(OnlyPartial);
 }
 
 llvm::FoldingSetVector<VarTemplateSpecializationDecl> &
@@ -1237,7 +1287,7 @@ VarTemplateDecl::getSpecializations() const {
 
 llvm::FoldingSetVector<VarTemplatePartialSpecializationDecl> &
 VarTemplateDecl::getPartialSpecializations() const {
-  LoadLazySpecializations();
+  LoadLazySpecializations(/*PartialOnly = */ true);
   return getCommonPtr()->PartialSpecializations;
 }
 
@@ -1251,12 +1301,14 @@ VarTemplateDecl::newCommon(ASTContext &C) const {
 VarTemplateSpecializationDecl *
 VarTemplateDecl::findSpecialization(ArrayRef<TemplateArgument> Args,
                                     void *&InsertPos) {
-  return findSpecializationImpl(getSpecializations(), InsertPos, Args);
+  auto *Common = getCommonPtr();
+  return findSpecializationImpl(Common->Specializations, InsertPos, Args);
 }
 
 void VarTemplateDecl::AddSpecialization(VarTemplateSpecializationDecl *D,
                                         void *InsertPos) {
-  addSpecializationImpl<VarTemplateDecl>(getSpecializations(), D, InsertPos);
+  auto *Common = getCommonPtr();
+  addSpecializationImpl<VarTemplateDecl>(Common->Specializations, D, InsertPos);
 }
 
 VarTemplatePartialSpecializationDecl *
diff --git a/clang/lib/AST/ODRHash.cpp b/clang/lib/AST/ODRHash.cpp
index 2dbc259138a897..ffc48ee7cb95ac 100644
--- a/clang/lib/AST/ODRHash.cpp
+++ b/clang/lib/AST/ODRHash.cpp
@@ -812,6 +812,21 @@ void ODRHash::AddDecl(const Decl *D) {
     for (const TemplateArgument &TA : List.asArray())
       AddTemplateArgument(TA);
   }
+
+  // If this was a specialization we should take into account its template
+  // arguments. This helps to reduce collisions coming when visiting template
+  // specialization types (eg. when processing type template arguments).
+  ArrayRef<TemplateArgument> Args;
+  if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(D))
+    Args = CTSD->getTemplateArgs().asArray();
+  else if (auto *VTSD = dyn_cast<VarTemplateSpecializationDecl>(D))
+    Args = VTSD->getTemplateArgs().asArray();
+  else if (auto *FD = dyn_cast<FunctionDecl>(D))
+    if (FD->getTemplateSpecializationArgs())
+      Args = FD->getTemplateSpecializationArgs()->asArray();
+
+  for (auto &TA : Args)
+    AddTemplateArgument(TA);
 }
 
 namespace {
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 683a076e6bc399..3a4fcc8c118fee 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -7549,14 +7549,23 @@ void ASTReader::CompleteRedeclChain(const Decl *D) {
     }
   }
 
-  if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(D))
-    CTSD->getSpecializedTemplate()->LoadLazySpecializations();
-  if (auto *VTSD = dyn_cast<VarTemplateSpecializationDecl>(D))
-    VTSD->getSpecializedTemplate()->LoadLazySpecializations();
-  if (auto *FD = dyn_cast<FunctionDecl>(D)) {
-    if (auto *Template = FD->getPrimaryTemplate())
-      Template->LoadLazySpecializations();
-  }
+  RedeclarableTemplateDecl *Template = nullptr;
+  ArrayRef<TemplateArgument> Args;
+  if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(D)) {
+    Template = CTSD->getSpecializedTemplate();
+    Args = CTSD->getTemplateArgs().asArray();
+  } else if (auto *VTSD = dyn_cast<VarTemplateSpecializationDecl>(D)) {
+    Template = VTSD->getSpecializedTemplate();
+    Args = VTSD->getTemplateArgs().asArray();
+  } else if (auto *FD = dyn_cast<FunctionDecl>(D)) {
+    if (auto *Tmplt = FD->getPrimaryTemplate()) {
+      Template = Tmplt;
+      Args = FD->getTemplateSpecializationArgs()->asArray();
+    }
+  }
+
+  if (Template)
+    Template->loadLazySpecializationsImpl(Args);
 }
 
 CXXCtorInitializer **
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index ffba04f28782ea..07d4fe81957d7c 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -88,6 +88,8 @@ namespace clang {
     const SourceLocation ThisDeclLoc;
 
     using RecordData = ASTReader::RecordData;
+    using LazySpecializationInfo
+      = RedeclarableTemplateDecl::LazySpecializationInfo;
 
     TypeID DeferredTypeID = 0;
     unsigned AnonymousDeclNumber = 0;
@@ -134,9 +136,16 @@ namespace clang {
       return Record.readString();
     }
 
-    void readDeclIDList(SmallVectorImpl<DeclID> &IDs) {
+    LazySpecializationInfo ReadLazySpecializationInfo() {
+      DeclID ID = readDeclID();
+      unsigned Hash = Record.readInt();
+      bool IsPartial = Record.readInt();
+      return LazySpecializationInfo(ID, Hash, IsPartial);
+    }
+
+    void readDeclIDList(SmallVectorImpl<LazySpecializationInfo> &IDs) {
       for (unsigned I = 0, Size = Record.readInt(); I != Size; ++I)
-        IDs.push_back(readDeclID());
+        IDs.push_back(ReadLazySpecializationInfo());
     }
 
     Decl *readDecl() {
@@ -267,7 +276,7 @@ namespace clang {
 
     template <typename T> static
     void AddLazySpecializations(T *D,
-                                SmallVectorImpl<serialization::DeclID>& IDs) {
+                                SmallVectorImpl<LazySpecializationInfo>& IDs) {
       if (IDs.empty())
         return;
 
@@ -277,12 +286,11 @@ namespace clang {
       auto *&LazySpecializations = D->getCommonPtr()->LazySpecializations;
 
       if (auto &Old = LazySpecializations) {
-        IDs.insert(IDs.end(), Old + 1, Old + 1 + Old[0]);
+        IDs.insert(IDs.end(), Old + 1, Old + 1 + Old[0].DeclID);
         llvm::sort(IDs);
         IDs.erase(std::unique(IDs.begin(), IDs.end()), IDs.end());
       }
-
-      auto *Result = new (C) serialization::DeclID[1 + IDs.size()];
+      auto *Result = new (C) LazySpecializationInfo[1 + IDs.size()];
       *Result = IDs.size();
       std::copy(IDs.begin(), IDs.end(), Result + 1);
 
@@ -320,7 +328,7 @@ namespace clang {
     void ReadFunctionDefinition(FunctionDecl *FD);
     void Visit(Decl *D);
 
-    void UpdateDecl(Decl *D, SmallVectorImpl<serialization::DeclID> &);
+    void UpdateDecl(Decl *D, llvm::SmallVectorImpl<LazySpecializationInfo>&);
 
     static void setNextObjCCategory(ObjCCategoryDecl *Cat,
                                     ObjCCategoryDecl *Next) {
@@ -2450,7 +2458,7 @@ void ASTDeclReader::VisitClassTemplateDecl(ClassTemplateDecl *D) {
   if (ThisDeclID == Redecl.getFirstID()) {
     // This ClassTemplateDecl owns a CommonPtr; read it to keep track of all of
     // the specializations.
-    SmallVector<serialization::DeclID, 32> SpecIDs;
+    SmallVector<LazySpecializationInfo, 32> SpecIDs;
     readDeclIDList(SpecIDs);
     ASTDeclReader::AddLazySpecializations(D, SpecIDs);
   }
@@ -2478,7 +2486,7 @@ void ASTDeclReader::VisitVarTemplateDecl(VarTemplateDecl *D) {
   if (ThisDeclID == Redecl.getFirstID()) {
     // This VarTemplateDecl owns a CommonPtr; read it to keep track of all of
     // the specializations.
-    SmallVector<serialization::DeclID, 32> SpecIDs;
+    SmallVector<LazySpecializationInfo, 32> SpecIDs;
     readDeclIDList(SpecIDs);
     ASTDeclReader::AddLazySpecializations(D, SpecIDs);
   }
@@ -2580,7 +2588,7 @@ void ASTDeclReader::VisitFunctionTemplateDecl(FunctionTemplateDecl *D) {
 
   if (ThisDeclID == Redecl.getFirstID()) {
     // This FunctionTemplateDecl owns a CommonPtr; read it.
-    SmallVector<serialization::DeclID, 32> SpecIDs;
+    SmallVector<LazySpecializationInfo, 32> SpecIDs;
     readDeclIDList(SpecIDs);
     ASTDeclReader::AddLazySpecializations(D, SpecIDs);
   }
@@ -4191,7 +4199,9 @@ void ASTReader::loadDeclUpdateRecords(PendingUpdateRecord &Record) {
   ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
   DeclUpdateOffsetsMap::iterator UpdI = DeclUpdateOffsets.find(ID);
 
-  SmallVector<serialization::DeclID, 8> PendingLazySpecializationIDs;
+  using LazySpecializationInfo
+    = RedeclarableTemplateDecl::LazySpecializationInfo;
+  llvm::SmallVector<LazySpecializationInfo, 8> PendingLazySpecializationIDs;
 
   if (UpdI != DeclUpdateOffsets.end()) {
     auto UpdateOffsets = std::move(UpdI->second);
@@ -4461,7 +4471,7 @@ static void forAllLaterRedecls(DeclT *D, Fn F) {
 }
 
 void ASTDeclReader::UpdateDecl(Decl *D,
-   llvm::SmallVectorImpl<serialization::DeclID> &PendingLazySpecializationIDs) {
+        SmallVectorImpl<LazySpecializationInfo> &PendingLazySpecializationIDs) {
   while (Record.getIdx() < Record.size()) {
     switch ((DeclUpdateKind)Record.readInt()) {
     case UPD_CXX_ADDED_IMPLICIT_MEMBER: {
@@ -4474,7 +4484,7 @@ void ASTDeclReader::UpdateDecl(Decl *D,
 
     case UPD_CXX_ADDED_TEMPLATE_SPECIALIZATION:
       // It will be added to the template's lazy specialization set.
-      PendingLazySpecializationIDs.push_back(readDeclID());
+      PendingLazySpecializationIDs.push_back(ReadLazySpecializationInfo());
       break;
 
     case UPD_CXX_ADDED_ANONYMOUS_NAMESPACE: {
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 740bec586a5e33..6cb6d3d8fae4d8 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -5329,12 +5329,29 @@ void ASTWriter::WriteDeclUpdatesBlocks(RecordDataImpl &OffsetsRecord) {
 
       switch (Kind) {
       case UPD_CXX_ADDED_IMPLICIT_MEMBER:
-      case UPD_CXX_ADDED_TEMPLATE_SPECIALIZATION:
       case UPD_CXX_ADDED_ANONYMOUS_NAMESPACE:
         assert(Update.getDecl() && "no decl to add?");
         Record.push_back(GetDeclRef(Update.getDecl()));
         break;
-
+  ...
[truncated]

@ChuanqiXu9
Copy link
Member Author

ChuanqiXu9 commented Feb 27, 2024

Personally I feel this patch is good and the testing result from our workload shows it is good too. But it looks like the performance testing results from google @zygoloid @ilya-biryukov was not good. So maybe we need to wait that for landing this. (It will be great if @ilya-biryukov would like to test again)

Copy link

github-actions bot commented Feb 27, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 62a7562b828ee9df6a41b6ce3dfda7c5511816a6 45ed7aa243654a114e326538fd5cdc5096df1b35 -- clang/include/clang/AST/DeclTemplate.h clang/lib/AST/DeclTemplate.cpp clang/lib/AST/ODRHash.cpp clang/lib/Serialization/ASTReader.cpp clang/lib/Serialization/ASTReaderDecl.cpp clang/lib/Serialization/ASTWriter.cpp clang/lib/Serialization/ASTWriterDecl.cpp clang/test/Modules/cxx-templates.cpp clang/test/Modules/odr_hash.cpp
View the diff from clang-format here.
diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp
index a6381d78cd..47adb4702d 100644
--- a/clang/lib/AST/DeclTemplate.cpp
+++ b/clang/lib/AST/DeclTemplate.cpp
@@ -369,8 +369,8 @@ void RedeclarableTemplateDecl::loadLazySpecializationsImpl(
     unsigned Hash = TemplateArgumentList::ComputeODRHash(Args);
     unsigned N = Specs[0].DeclID.getRawValue();
     for (unsigned I = 0; I != N; ++I)
-      if (Specs[I+1].ODRHash && Specs[I+1].ODRHash == Hash)
-        (void)loadLazySpecializationImpl(Specs[I+1]);
+      if (Specs[I + 1].ODRHash && Specs[I + 1].ODRHash == Hash)
+        (void)loadLazySpecializationImpl(Specs[I + 1]);
   }
 }
 
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 1ab2c38007..a8b02a8a33 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -268,7 +268,8 @@ namespace clang {
           RedeclarableTemplateDecl::LazySpecializationInfo;
       ArrayRef<LazySpecializationInfo> LazySpecializations;
       if (auto *LS = Common->LazySpecializations)
-        LazySpecializations = llvm::ArrayRef(LS + 1, LS[0].DeclID.getRawValue());
+        LazySpecializations =
+            llvm::ArrayRef(LS + 1, LS[0].DeclID.getRawValue());
 
       // Add a slot to the record for the number of specializations.
       unsigned I = Record.size();

@vgvassilev
Copy link
Contributor

Personally I feel this patch is good and the testing result from our workload shows it is good too. But it looks like the performance testing results from google @zygoloid @ilya-biryukov is not good. So maybe we need to wait for landing this. (It will be great if @ilya-biryukov would like to test again)

Thanks for putting this up. I was planning to do so, however, locally I got:

  Clang :: CXX/module/basic/basic.link/p2.cppm
  Clang :: Driver/clang_f_opts.c
  Clang :: Driver/darwin-header-search-libcxx.cpp
  Clang :: Index/crash-recovery-modules.m
  Clang :: Modules/ExtDebugInfo.cpp
  Clang :: Modules/cxx-templates.cpp
  Clang :: Modules/odr_hash.cpp
  Clang :: Modules/using-directive-redecl.cpp
  Clang :: Modules/using-directive.cpp
  Clang :: PCH/chain-late-anonymous-namespace.cpp
  Clang :: PCH/cxx-namespaces.cpp
  Clang :: PCH/namespaces.cpp

I believe some tests require just adjusting the reference files, but others may need some debugging.

@ChuanqiXu9
Copy link
Member Author

Weird. I only see two failures in my local environment:

Failed Tests (2):
  Clang :: Modules/cxx-templates.cpp
  Clang :: Modules/odr_hash.cpp

And I saw both of them in my patch. It is simply order mismatches.

@vgvassilev
Copy link
Contributor

Weird. I only see two failures in my local environment:

Failed Tests (2):
  Clang :: Modules/cxx-templates.cpp
  Clang :: Modules/odr_hash.cpp

And I saw both of them in my patch. It is simply order mismatches.

Ha, ok. I know that my system constantly have some tests that fail even fro the master but I thought these were real. If we don't see them in the bots then that's probably a problem only on my side...

ChuanqiXu9 added a commit that referenced this pull request Feb 28, 2024
Following up for #83108

This follows the suggestion literally from
#76774 (comment)

which introduces OnDiskHashTable for specializations based on D41416.

Note that I didn't polish this patch to reduce the diff from D41416 to
it easier to review. I'll make the polishing patch later. So that we can
focus what we're doing in this patch and focus on the style in the next
patch.
ChuanqiXu9 added a commit that referenced this pull request Mar 5, 2024
Following up for #83108

This follows the suggestion literally from
#76774 (comment)

which introduces OnDiskHashTable for specializations based on D41416.

Note that I didn't polish this patch to reduce the diff from D41416 to
it easier to review. I'll make the polishing patch later. So that we can
focus what we're doing in this patch and focus on the style in the next
patch.
ChuanqiXu9 added a commit to ChuanqiXu9/llvm-project that referenced this pull request Apr 7, 2024
Following up for llvm#83108

This follows the suggestion literally from
llvm#76774 (comment)

which introduces OnDiskHashTable for specializations based on D41416.

Note that I didn't polish this patch to reduce the diff from D41416 to
it easier to review. I'll make the polishing patch later. So that we can
focus what we're doing in this patch and focus on the style in the next
patch.
@ChuanqiXu9
Copy link
Member Author

Rebased with main.

ChuanqiXu9 added a commit that referenced this pull request Apr 25, 2024
Following up for #83108

This follows the suggestion literally from
#76774 (comment)

which introduces OnDiskHashTable for specializations based on
D41416.

Note that I didn't polish this patch to reduce the diff from D41416
to it easier to review. I'll make the polishing patch later. So that we
can focus what we're doing in this patch and focus on the style in the
next patch.
ChuanqiXu9 added a commit that referenced this pull request Jul 5, 2024
Following up for #83108

This follows the suggestion literally from
#76774 (comment)

which introduces OnDiskHashTable for specializations based on
D41416.

Note that I didn't polish this patch to reduce the diff from D41416
to it easier to review. I'll make the polishing patch later. So that we
can focus what we're doing in this patch and focus on the style in the
next patch.
ChuanqiXu9 added a commit to ChuanqiXu9/llvm-project that referenced this pull request Jul 10, 2024
Following up for llvm#83108

This follows the suggestion literally from
llvm#76774 (comment)

which introduces OnDiskHashTable for specializations based on
D41416.

Note that I didn't polish this patch to reduce the diff from D41416
to it easier to review. I'll make the polishing patch later. So that we
can focus what we're doing in this patch and focus on the style in the
next patch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants