[Serialization] Code cleanups and polish 83233 #83237
Closed
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-modules @llvm/pr-subscribers-clang Author: Chuanqi Xu (ChuanqiXu9) ChangesThis is the following of #83233. This simply removes Patch is 21.06 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/83237.diff 8 Files Affected:
diff --git a/clang/include/clang/AST/DeclTemplate.h b/clang/include/clang/AST/DeclTemplate.h
index 51caef54baac26..d25e10adb5453d 100644
--- a/clang/include/clang/AST/DeclTemplate.h
+++ b/clang/include/clang/AST/DeclTemplate.h
@@ -256,8 +256,8 @@ 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 stable hash for the given arguments across compiler invocations.
+ static unsigned ComputeStableHash(ArrayRef<TemplateArgument> Args);
/// Create a new template argument list that copies the given set of
/// template arguments.
@@ -733,25 +733,6 @@ 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 {
@@ -798,8 +779,6 @@ class RedeclarableTemplateDecl : public TemplateDecl,
void loadLazySpecializationsImpl(llvm::ArrayRef<TemplateArgument> Args,
TemplateParameterList *TPL = nullptr) const;
- Decl *loadLazySpecializationImpl(LazySpecializationInfo &LazySpecInfo) const;
-
template <class EntryType, typename ...ProfileArguments>
typename SpecEntryTraits<EntryType>::DeclType*
findSpecializationImpl(llvm::FoldingSetVector<EntryType> &Specs,
@@ -820,13 +799,6 @@ class RedeclarableTemplateDecl : public TemplateDecl,
llvm::PointerIntPair<RedeclarableTemplateDecl*, 1, bool>
InstantiatedFromMember;
- /// If non-null, points to an array of specializations (including
- /// partial specializations) known only by their external declaration IDs.
- ///
- /// The first value in the array is the number of specializations/partial
- /// specializations that follow.
- LazySpecializationInfo *LazySpecializations = nullptr;
-
/// The set of "injected" template arguments used within this
/// template.
///
diff --git a/clang/include/clang/Serialization/ASTBitCodes.h b/clang/include/clang/Serialization/ASTBitCodes.h
index 15e7aef826a52a..827799ec7f0a3b 100644
--- a/clang/include/clang/Serialization/ASTBitCodes.h
+++ b/clang/include/clang/Serialization/ASTBitCodes.h
@@ -699,7 +699,7 @@ enum ASTRecordTypes {
/// recorded in a preamble.
PP_ASSUME_NONNULL_LOC = 67,
- UPDATE_SPECIALIZATION = 68,
+ CXX_ADDED_TEMPLATE_SPECIALIZATION = 68,
};
/// Record types used within a source manager block.
diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp
index 7b98b046d00725..4115f8ae4f0382 100644
--- a/clang/lib/AST/DeclTemplate.cpp
+++ b/clang/lib/AST/DeclTemplate.cpp
@@ -342,32 +342,6 @@ void RedeclarableTemplateDecl::loadLazySpecializationsImpl(
ExternalSource->LoadExternalSpecializations(this->getCanonicalDecl(),
OnlyPartial);
return;
-
- // Grab the most recent declaration to ensure we've loaded any lazy
- // redeclarations of this template.
- CommonBase *CommonBasePtr = getMostRecentDecl()->getCommonPtr();
- 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 {
- llvm_unreachable("We don't use LazySpecializationInfo any more");
-
- 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(
@@ -378,14 +352,6 @@ void RedeclarableTemplateDecl::loadLazySpecializationsImpl(
ExternalSource->LoadExternalSpecializations(this->getCanonicalDecl(), Args);
return;
-
- 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]);
- }
}
template<class EntryType, typename... ProfileArguments>
@@ -939,7 +905,20 @@ TemplateArgumentList::CreateCopy(ASTContext &Context,
return new (Mem) TemplateArgumentList(Args);
}
-unsigned TemplateArgumentList::ComputeODRHash(ArrayRef<TemplateArgument> Args) {
+unsigned
+TemplateArgumentList::ComputeStableHash(ArrayRef<TemplateArgument> Args) {
+ // FIXME: ODR hashing may not be the best mechanism to hash the template
+ // arguments. ODR hashing is (or perhaps, should be) about determining whether
+ // two things are spelled the same way and have the same meaning (as required
+ // by the C++ ODR), whereas what we want here is whether they have the same
+ // meaning regardless of spelling. Maybe we can get away with reusing ODR
+ // hashing anyway, on the basis that any canonical, non-dependent template
+ // argument should have the same (invented) spelling in every translation
+ // unit, but it is not sure that's true in all cases. There may still be cases
+ // where the canonical type includes some aspect of "whatever we saw first",
+ // in which case the ODR hash can differ across translation units for
+ // non-dependent, canonical template arguments that are spelled differently
+ // but have the same meaning. But it is not easy to raise examples.
ODRHash Hasher;
for (TemplateArgument TA : Args)
Hasher.AddTemplateArgument(TA);
diff --git a/clang/lib/Serialization/ASTCommon.h b/clang/lib/Serialization/ASTCommon.h
index 296642e3674a49..485809f234113f 100644
--- a/clang/lib/Serialization/ASTCommon.h
+++ b/clang/lib/Serialization/ASTCommon.h
@@ -23,7 +23,6 @@ namespace serialization {
enum DeclUpdateKind {
UPD_CXX_ADDED_IMPLICIT_MEMBER,
- UPD_CXX_ADDED_TEMPLATE_SPECIALIZATION,
UPD_CXX_ADDED_ANONYMOUS_NAMESPACE,
UPD_CXX_ADDED_FUNCTION_DEFINITION,
UPD_CXX_ADDED_VAR_DEFINITION,
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 0c6c19dfe5f908..94d7b959bcfdcc 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -3497,7 +3497,7 @@ llvm::Error ASTReader::ReadASTBlock(ModuleFile &F,
break;
}
- case UPDATE_SPECIALIZATION: {
+ case CXX_ADDED_TEMPLATE_SPECIALIZATION: {
unsigned Idx = 0;
serialization::DeclID ID = ReadDeclID(F, Record, Idx);
auto *Data = (const unsigned char *)Blob.data();
@@ -7972,7 +7972,7 @@ void ASTReader::LoadExternalSpecializations(
return;
Deserializing LookupResults(this);
- auto HashValue = TemplateArgumentList::ComputeODRHash(TemplateArgs);
+ auto HashValue = TemplateArgumentList::ComputeStableHash(TemplateArgs);
// Get Decl may violate the iterator from SpecializationsLookups
llvm::SmallVector<serialization::reader::LazySpecializationInfo, 8> Infos;
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 0b42b4c2713162..6236795dcd4156 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -88,8 +88,6 @@ namespace clang {
const SourceLocation ThisDeclLoc;
using RecordData = ASTReader::RecordData;
- using LazySpecializationInfo =
- RedeclarableTemplateDecl::LazySpecializationInfo;
TypeID DeferredTypeID = 0;
unsigned AnonymousDeclNumber = 0;
@@ -136,18 +134,6 @@ namespace clang {
return Record.readString();
}
- 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(ReadLazySpecializationInfo());
- }
-
Decl *readDecl() {
return Record.readDecl();
}
@@ -274,29 +260,6 @@ namespace clang {
: Reader(Reader), Record(Record), Loc(Loc), ThisDeclID(thisDeclID),
ThisDeclLoc(ThisDeclLoc) {}
- template <typename T>
- static void
- AddLazySpecializations(T *D, SmallVectorImpl<LazySpecializationInfo> &IDs) {
- if (IDs.empty())
- return;
-
- // FIXME: We should avoid this pattern of getting the ASTContext.
- ASTContext &C = D->getASTContext();
-
- auto *&LazySpecializations = D->getCommonPtr()->LazySpecializations;
-
- if (auto &Old = LazySpecializations) {
- 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) LazySpecializationInfo[1 + IDs.size()];
- *Result = IDs.size();
- std::copy(IDs.begin(), IDs.end(), Result + 1);
-
- LazySpecializations = Result;
- }
-
template <typename DeclT>
static Decl *getMostRecentDeclImpl(Redeclarable<DeclT> *D);
static Decl *getMostRecentDeclImpl(...);
@@ -328,7 +291,7 @@ namespace clang {
void ReadFunctionDefinition(FunctionDecl *FD);
void Visit(Decl *D);
- void UpdateDecl(Decl *D, llvm::SmallVectorImpl<LazySpecializationInfo> &);
+ void UpdateDecl(Decl *D);
static void setNextObjCCategory(ObjCCategoryDecl *Cat,
ObjCCategoryDecl *Next) {
@@ -2469,9 +2432,6 @@ 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<LazySpecializationInfo, 32> SpecIDs;
- readDeclIDList(SpecIDs);
- ASTDeclReader::AddLazySpecializations(D, SpecIDs);
ReadSpecializations(*Loc.F, D, Loc.F->DeclsCursor);
}
@@ -2498,9 +2458,6 @@ 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<LazySpecializationInfo, 32> SpecIDs;
- readDeclIDList(SpecIDs);
- ASTDeclReader::AddLazySpecializations(D, SpecIDs);
ReadSpecializations(*Loc.F, D, Loc.F->DeclsCursor);
}
}
@@ -2601,9 +2558,6 @@ void ASTDeclReader::VisitFunctionTemplateDecl(FunctionTemplateDecl *D) {
if (ThisDeclID == Redecl.getFirstID()) {
// This FunctionTemplateDecl owns a CommonPtr; read it.
- SmallVector<LazySpecializationInfo, 32> SpecIDs;
- readDeclIDList(SpecIDs);
- ASTDeclReader::AddLazySpecializations(D, SpecIDs);
ReadSpecializations(*Loc.F, D, Loc.F->DeclsCursor);
}
}
@@ -4214,10 +4168,6 @@ void ASTReader::loadDeclUpdateRecords(PendingUpdateRecord &Record) {
ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
DeclUpdateOffsetsMap::iterator UpdI = DeclUpdateOffsets.find(ID);
- using LazySpecializationInfo =
- RedeclarableTemplateDecl::LazySpecializationInfo;
- llvm::SmallVector<LazySpecializationInfo, 8> PendingLazySpecializationIDs;
-
if (UpdI != DeclUpdateOffsets.end()) {
auto UpdateOffsets = std::move(UpdI->second);
DeclUpdateOffsets.erase(UpdI);
@@ -4255,7 +4205,7 @@ void ASTReader::loadDeclUpdateRecords(PendingUpdateRecord &Record) {
ASTDeclReader Reader(*this, Record, RecordLocation(F, Offset), ID,
SourceLocation());
- Reader.UpdateDecl(D, PendingLazySpecializationIDs);
+ Reader.UpdateDecl(D);
// We might have made this declaration interesting. If so, remember that
// we need to hand it off to the consumer.
@@ -4267,19 +4217,6 @@ void ASTReader::loadDeclUpdateRecords(PendingUpdateRecord &Record) {
}
}
}
- // Add the lazy specializations to the template.
- assert((PendingLazySpecializationIDs.empty() || isa<ClassTemplateDecl>(D) ||
- isa<FunctionTemplateDecl, VarTemplateDecl>(D)) &&
- "Must not have pending specializations");
- /*
- if (auto *CTD = dyn_cast<ClassTemplateDecl>(D))
- ASTDeclReader::AddLazySpecializations(CTD, PendingLazySpecializationIDs);
- else if (auto *FTD = dyn_cast<FunctionTemplateDecl>(D))
- ASTDeclReader::AddLazySpecializations(FTD, PendingLazySpecializationIDs);
- else if (auto *VTD = dyn_cast<VarTemplateDecl>(D))
- ASTDeclReader::AddLazySpecializations(VTD, PendingLazySpecializationIDs);
- */
- PendingLazySpecializationIDs.clear();
// Load the pending visible updates for this decl context, if it has any.
auto I = PendingVisibleUpdates.find(ID);
@@ -4497,9 +4434,7 @@ static void forAllLaterRedecls(DeclT *D, Fn F) {
}
}
-void ASTDeclReader::UpdateDecl(
- Decl *D,
- SmallVectorImpl<LazySpecializationInfo> &PendingLazySpecializationIDs) {
+void ASTDeclReader::UpdateDecl(Decl *D) {
while (Record.getIdx() < Record.size()) {
switch ((DeclUpdateKind)Record.readInt()) {
case UPD_CXX_ADDED_IMPLICIT_MEMBER: {
@@ -4510,11 +4445,6 @@ void ASTDeclReader::UpdateDecl(
break;
}
- case UPD_CXX_ADDED_TEMPLATE_SPECIALIZATION:
- // It will be added to the template's lazy specialization set.
- PendingLazySpecializationIDs.push_back(ReadLazySpecializationInfo());
- break;
-
case UPD_CXX_ADDED_ANONYMOUS_NAMESPACE: {
auto *Anon = readDeclAs<NamespaceDecl>();
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 20d5a357647a75..0f217919ce3993 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -4044,7 +4044,7 @@ unsigned CalculateODRHashForSpecs(const Decl *Spec) {
else
llvm_unreachable("New Specialization Kind?");
- return TemplateArgumentList::ComputeODRHash(Args);
+ return TemplateArgumentList::ComputeStableHash(Args);
}
} // namespace
@@ -5453,7 +5453,7 @@ ASTFileSignature ASTWriter::WriteASTCore(Sema &SemaRef, StringRef isysroot,
void ASTWriter::WriteSpecializationsUpdates() {
auto Abv = std::make_shared<llvm::BitCodeAbbrev>();
- Abv->Add(llvm::BitCodeAbbrevOp(UPDATE_SPECIALIZATION));
+ Abv->Add(llvm::BitCodeAbbrevOp(CXX_ADDED_TEMPLATE_SPECIALIZATION));
Abv->Add(llvm::BitCodeAbbrevOp(llvm::BitCodeAbbrevOp::VBR, 6));
Abv->Add(llvm::BitCodeAbbrevOp(llvm::BitCodeAbbrevOp::Blob));
auto UpdateSpecializationAbbrev = Stream.EmitAbbrev(std::move(Abv));
@@ -5466,7 +5466,8 @@ void ASTWriter::WriteSpecializationsUpdates() {
LookupTable);
// Write the lookup table
- RecordData::value_type Record[] = {UPDATE_SPECIALIZATION, getDeclID(D)};
+ RecordData::value_type Record[] = {CXX_ADDED_TEMPLATE_SPECIALIZATION,
+ getDeclID(D)};
Stream.EmitRecordWithBlob(UpdateSpecializationAbbrev, Record, LookupTable);
}
}
@@ -5503,25 +5504,6 @@ void ASTWriter::WriteDeclUpdatesBlocks(RecordDataImpl &OffsetsRecord) {
assert(Update.getDecl() && "no decl to add?");
Record.push_back(GetDeclRef(Update.getDecl()));
break;
- case UPD_CXX_ADDED_TEMPLATE_SPECIALIZATION: {
- const Decl *Spec = Update.getDecl();
- assert(Spec && "no decl to add?");
- Record.push_back(GetDeclRef(Spec));
- ArrayRef<TemplateArgument> Args;
- if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(Spec))
- Args = CTSD->getTemplateArgs().asArray();
- else if (auto *VTSD = dyn_cast<VarTemplateSpecializationDecl>(Spec))
- Args = VTSD->getTemplateArgs().asArray();
- else if (auto *FD = dyn_cast<FunctionDecl>(Spec))
- Args = FD->getTemplateSpecializationArgs()->asArray();
- assert(Args.size());
- Record.push_back(TemplateArgumentList::ComputeODRHash(Args));
- bool IsPartialSpecialization =
- isa<ClassTemplatePartialSpecializationDecl>(Spec) ||
- isa<VarTemplatePartialSpecializationDecl>(Spec);
- Record.push_back(IsPartialSpecialization);
- break;
- }
case UPD_CXX_ADDED_FUNCTION_DEFINITION:
case UPD_CXX_ADDED_VAR_DEFINITION:
break;
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index f021d82a6d2409..30c1ef403ecf01 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -213,24 +213,8 @@ namespace clang {
llvm::MapVector<ModuleFile *, const Decl *> Firsts;
CollectFirstDeclFromEachModule(D, /*IncludeLocal*/ true, Firsts);
- for (const auto &F : Firsts) {
+ for (const auto &F : Firsts)
SpecsInMap.push_back(F.second);
-
- Record.AddDeclRef(F.second);
- 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))
- Args = FD->getTemplateSpecializationArgs()->asArray();
- assert(Args.size());
- Record.push_back(TemplateArgumentList::ComputeODRHash(Args));
- bool IsPartialSpecialization =
- isa<ClassTemplatePartialSpecializationDecl>(D) ||
- isa<VarTemplatePartialSpecializationDecl>(D);
- Record.push_back(IsPartialSpecialization);
- }
}
/// Get the specialization decl from an entry in the specialization list.
@@ -257,22 +241,12 @@ namespace clang {
// If we have any lazy specializations, and the external AST source is
// our chained AST reader, we can just write out the DeclIDs. Otherwise,
// we need to resolve them to actual declarations.
- if (Writer.Chain != Writer.Context->getExternalSource() &&
- Common->LazySpecializations) {
+ if (Writer.Chain != Writer.Context->getExternalSource() && Writer.Chain &&
+ Writer.Chain->getLoadedSpecializationsLookupTables(D)) {
D->LoadLazySpecializations();
- assert(!Common->LazySpecializations);
+ assert(!Writer.Chain->getLoadedSpecializationsLookupTables(D));
}
- using LazySpecializationInfo =
- RedeclarableTemplateDecl::LazySpecializationInfo;
- ArrayRef<LazySpecializationInfo> LazySpecializations;
- if (auto *LS = Common->LazySpecializations)
- LazySpecializations = llvm::ArrayRef(LS + 1, LS[0].DeclID);
-
- // Add a slot to the record for the number of specializations.
- unsigned I = Record.size();
- Record.push_back(0);
-
// AddFirstDeclFromEachModule might trigger deserialization, invalidating
// *Specializations iterators.
llvm::SmallVector<const Decl*, 16> Specs;
@@ -288,21 +262,6 @@ namespace clang {
AddFirstSpecializationDeclFromEachModule(D, SpecsInOnDiskMap);
}
- // We don't need to insert LazySpecializations to SpecsInOnDiskMap,
- // since we'll handle that in GenerateSpeciali...
[truncated]
|
ChuanqiXu9
commented
Feb 28, 2024
Comment on lines
244
to
256
| if (Writer.Chain != Writer.Context->getExternalSource() && Writer.Chain && | ||
| Writer.Chain->getLoadedSpecializationsLookupTables(D)) { | ||
| D->LoadLazySpecializations(); | ||
| assert(!Common->LazySpecializations); | ||
| assert(!Writer.Chain->getLoadedSpecializationsLookupTables(D)); | ||
| } |
There was a problem hiding this comment.
During the process of rewriting, I just realized that this may be a significant change between D41416 and my original patch. In my original patch, I feel this is redundant since I think https://github.com/llvm/llvm-project/pull/83233/files#diff-6fe53759e8d797c328c73ada5f3324c6248a8634ef36131c7eb2b9d89192bb64R4082-R4084 can handle the external specialization. But this is not true. The process of loading specializations has a strong side effect that it may trigger more deserialization and probably loading other specializations. However, the process of re-inserting the OnDiskHashTable won't trigger deserialization. It simply moves stored hash value and the DeclID.
I guess this may be the problem why my original patch can't pass the workloads in google and ROOT. If it is the case, it shows that we lack in tree test cases for that.
|
@vgvassilev this may be ready to test. |
I triggered a test here: root-project/root#14495 I still need to verify if I did not screw up bringing your changes back to our builds but locally it crashes quite early with: |
|
The error message looks odd since the language options shouldn't be involved. |
Sorry, I did not push. Now you can take a look. |
I mean your local results. |
ChuanqiXu9
force-pushed
the
users/ChuanqiXu9/D41416_on_disk_hash_table
branch
from
9a88b07 to
03e7d56
Compare
ChuanqiXu9
force-pushed
the
users/ChuanqiXu9/D41416_on_disk_hash_table_polishing
branch
2 times, most recently
from
19617bb to
1d288e7
Compare
|
@ilya-biryukov hi, the functional and performance test on the root side looks good: root-project/root#14495 (comment) Could you try to test this within google internals? Also if your projects implements new ExternalASTSource, you need to handle that like I did in root: ChuanqiXu9/root@570fd78 |
@ilya-biryukov, can you test this PR on your side. We'd like to move forward. cc: @dwblaikie |
|
We'll pick this up, I'll get back with the testing results. |
|
Btw, if I don't respond timely to these requests and you need an urgent reaction, feel free to ping me via email at [email protected]. |
@ilya-biryukov, I was wondering if you have had a chance to test this PR? |
|
@vgvassilev BTW, I found all of pages: #83108, #83233 and #83237 doesn't say here is a conflict. Do we need to rebase in this case? |
|
Sorry for the delay, I had an emergency and took a week off. // RUN: rm -rf %t
// RUN: mkdir -p %t
// RUN: split-file %s %t
//
// RUN: %clang_cc1 -std=c++20 %t/type_traits.cppm -emit-module-interface -o %t/type_traits.pcm
// RUN: %clang_cc1 -std=c++20 %t/test.cpp -fprebuilt-module-path=%t -verify
//--- type_traits.cppm
export module type_traits;
export template <typename T>
constexpr bool is_pod_v = __is_pod(T);
//--- test.cpp
import type_traits;
// Base is either void or wrapper<T>.
template <class Base> struct wrapper : Base {};
template <> struct wrapper<void> {};
// wrap<0>::type<T> is wrapper<T>, wrap<1>::type<T> is wrapper<wrapper<T>>,
// and so on.
template <int N>
struct wrap {
template <class Base>
using type = wrapper<typename wrap<N-1>::template type<Base>>;
};
template <>
struct wrap<0> {
template <class Base>
using type = wrapper<Base>;
};
inline constexpr int kMaxRank = 40;
template <int N, class Base = void>
using rank = typename wrap<N>::template type<Base>;
using rank_selector_t = rank<40>;
static_assert(is_pod_v<rank_selector_t>, "Must be POD");There might be more problems as this one has been caught early and blocked further progress. |
ChuanqiXu9
force-pushed
the
users/ChuanqiXu9/D41416_on_disk_hash_table
branch
from
03e7d56 to
f87a54e
Compare
ChuanqiXu9
force-pushed
the
users/ChuanqiXu9/D41416_on_disk_hash_table_polishing
branch
from
1d288e7 to
859705c
Compare
|
Thanks for the reduced test case. It is pretty helpful. I've added it to the current patch. The root cause of the issue comes from an oversight in https://github.com/llvm/llvm-project/pull/83108/files#diff-dffd10772d07fb8c737cdf812839afa0173447c4812698c0c19618c34d92daddR806-R829. That we calculate the ODR Hash for class template specialization twice. Then for the example you described, the linear recursive computation becomes to somewhat similar to fibonacci computation: This test case passes quickly now after I remove the duplicated ODR computation. |
|
Thanks for fixing it quickly, we'll try the new patch and get back with the results. |
|
We have hit quite a few issues when trying this out. I have spent a day trying to reduce to a small repro that I can share, but haven't not fully succeeded yet, I will continue tomorrow.
|
|
Sorry, still struggling to get a small repro. The build graphs we have are quite large, unfortunately. |
|
Can you export all files into a standalone reproducer? I might be able to reduce an example. |
ChuanqiXu9
force-pushed
the
users/ChuanqiXu9/D41416_on_disk_hash_table
branch
from
f565dd3 to
1172643
Compare
ChuanqiXu9
force-pushed
the
users/ChuanqiXu9/D41416_on_disk_hash_table_polishing
branch
from
e975c2e to
4d5b958
Compare
|
@vgvassilev @ilya-biryukov @alexfh If I read correctly, the only blocker issue is the above reported performance issue. And I tried to split partial specialization from the full specialization table to avoid merging tables again and again. Could you please take another round of testing? Thanks in ahead. |
@alexfh is on vacation until tomorrow, but we've already asked him to run another round of testing. |
|
@alexfh gentle ping |
|
Thanks for bearing with us. The latest round of correctness testing came out positive. We have had one sleeper issue (the venerable "inline function undefined") pop up in the leaf of the build graph, but we managed to work around it. @alexfh is still running the benchmarks to see which effect it has on the compilation times. But we have not seen new timeouts during correctness testing, and it's already a very positive sign. |
|
Once again, thanks for bearing with us and addressing all the issues. PS please note that the resolution of our compile-time profiling instruments is not that great, we might not notice even something like a 10% regression in compile times. |
ilya-biryukov
approved these changes
Dec 5, 2024
Thank you very much! |
|
Note that I'll merge this and #83233 and #83108 into a commit and commit it directly to trunk. Since these prs are split initially to make it clear to be reviewed. But during the review, we always add changes to this PR. So the meaning of stacked PR in this series of patches is pretty questionable. So I feel it is better to merge these PRs into a single commit so that it is easier to be cherry-picked and reverted in my experience. I'll add a link to the PR page in the commit message. Thanks every one here again. |
ChuanqiXu9
added a commit
that referenced
this pull request
Dec 6, 2024
Currently all the specializations of a template (including
instantiation, specialization and partial specializations) will be
loaded at once if we want to instantiate another instance for the
template, or find instantiation for the template, or just want to
complete the redecl chain.
This means basically we need to load every specializations for the
template once the template declaration got loaded. This is bad since
when we load a specialization, we need to load all of its template
arguments. Then we have to deserialize a lot of unnecessary
declarations.
For example,
```
// M.cppm
export module M;
export template <class T>
class A {};
export class ShouldNotBeLoaded {};
export class Temp {
A<ShouldNotBeLoaded> AS;
};
// use.cpp
import M;
A<int> a;
```
We should a specialization ` A<ShouldNotBeLoaded>` in `M.cppm` and we
instantiate the template `A` in `use.cpp`. Then we will deserialize
`ShouldNotBeLoaded` surprisingly when compiling `use.cpp`. And this
patch tries to avoid that.
Given that the templates are heavily used in C++, this is a pain point
for the performance.
This patch adds MultiOnDiskHashTable for specializations in the
ASTReader. Then we will only deserialize the specializations with the
same template arguments. We made that by using ODRHash for the template
arguments as the key of the hash table.
To review this patch, I think `ASTReaderDecl::AddLazySpecializations`
may be a good entry point.
The patch was reviewed in
#83237 but that PR is a stacked
PR. But I feel the intention of the stacked PRs get lost during the
review process. So I feel it is better to merge the commits into a
single commit instead of merging them in the PR page. It is better for
us to cherry-pick and revert.
|
Sent b5bd192 |
This was referenced Dec 6, 2024
+100, this is definitely the right call. |
Not obvious why to me. Do lldb's tests somehow rely on the template specializations to be loaded at module import time? |
|
This also broke the build of https://lab.llvm.org/buildbot/#/builders/141/builds/4432. |
|
This also broke the Darwin LLDB bots: https://ci.swift.org/view/all/job/llvm.org/view/LLDB/job/lldb-cmake/ Given they've been red for 15h and broke various other bots, can you please revert while you investigate? |
|
Change b5bd192 broke llvm's windows bots. See : https://lab.llvm.org/buildbot/#/builders/63/builds/2944 and this patch is the only commit in the blame list. I reverted this patch in 12bdeba. Please reland the change after fixing the underlying issues. |
|
Sent #119333 It looks like the lldb's failure is from we forgot to update the ExternalASTConsumer (I met this the second time. I am wondering if we can make it more automatically). The other windows failure is a pattern match failure. |
ChuanqiXu9
added a commit
that referenced
this pull request
Dec 11, 2024
) Reland #83237 --- (Original comments) Currently all the specializations of a template (including instantiation, specialization and partial specializations) will be loaded at once if we want to instantiate another instance for the template, or find instantiation for the template, or just want to complete the redecl chain. This means basically we need to load every specializations for the template once the template declaration got loaded. This is bad since when we load a specialization, we need to load all of its template arguments. Then we have to deserialize a lot of unnecessary declarations. For example, ``` // M.cppm export module M; export template <class T> class A {}; export class ShouldNotBeLoaded {}; export class Temp { A<ShouldNotBeLoaded> AS; }; // use.cpp import M; A<int> a; ``` We have a specialization ` A<ShouldNotBeLoaded>` in `M.cppm` and we instantiate the template `A` in `use.cpp`. Then we will deserialize `ShouldNotBeLoaded` surprisingly when compiling `use.cpp`. And this patch tries to avoid that. Given that the templates are heavily used in C++, this is a pain point for the performance. This patch adds MultiOnDiskHashTable for specializations in the ASTReader. Then we will only deserialize the specializations with the same template arguments. We made that by using ODRHash for the template arguments as the key of the hash table. To review this patch, I think `ASTReaderDecl::AddLazySpecializations` may be a good entry point.
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.
This is the following of #83233. This simply removes
LazySpecializationInfofromRedeclarableTemplateDecl. I feel it can make the review process much more easier after splitting this.