[Serialization] Code cleanups and polish 83233 #83237
Open
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
251
| 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 ibiryukov@google.com. |
@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. |
Not really, this is why it's taking so long. Our infrastructure in that space is lacking, the issue is that the root case is not in one compilation step, but rather in some of the dependencies and the dependency graph for any of those problems is really large. We will get you a reproducer, please bear with us. Sorry for taking a long time. |
Back in the day when we were more active developing modules we wrote this tool: https://github.com/Teemperor/hippie It hooks to the system calls and copies all files involved in a crash/miscompilation under a common sysroot preserving the file structure. Maybe that'd help... |
I tried to review the patch purely but it looks like not easy to fix the failures without reproducers... for performances, I am wondering if we can improve it by caching the hash results. But it will be helpful if we can have profile data for that. A lot thanks for testing this. |
ChuanqiXu9
force-pushed
the
users/ChuanqiXu9/D41416_on_disk_hash_table
branch
from
f87a54e
to
80c9ab1
Compare
ChuanqiXu9
force-pushed
the
users/ChuanqiXu9/D41416_on_disk_hash_table_polishing
branch
from
859705c
to
d7a7273
Compare
|
Rebased with main |
|
Apologies for the delay with the reproducer. @ilya-biryukov is on vacation, but maybe @sam-mccall can help with an isolated test case. Either way it seems to be challenging. |
|
After endless hours and recompilations, I finally have this to share. module.tgz |
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.