llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) <details> <summary>Changes</summary> See https://clang.llvm.org/docs/StandardCPlusPlusModules.html#experimental-non-cascading-changes for the full background. In short, we want to cut off the dependencies to improve the recompilation time. And namespace is special, as the same namespace can occur in many TUs. This patch tries to clean up unneeded reference to namespace from other module file. The touched parts are: - When write on disk lookup table, don't write the merged table. This makes the ownership/dependency much cleaner. For performance, I think the intention was to make the cost of mergin table amortized. But in our internal workloads, I didn't observe any regression if I turn off writing the merged table. - When writing redeclarations, only write the ID of the first namespace and avoid referenece to all other previous namespaces. (I don't want to write the first namespace either... but it is more complex). - When writing the name lookup table, try to write the local namespace. @<!-- -->jtstogel @<!-- -->mpark I want to invite you to test this with your internal workloads to figure out the correctness and the performance impact on this. I know I can make the change clean for you by inserting code guarded by "if writing named module" but I think it will be much better if we can make the underlying implementation more homogeneous if possible. --- Full diff: https://github.com/llvm/llvm-project/pull/179178.diff 5 Files Affected: - (modified) clang/include/clang/Serialization/ASTWriter.h (-2) - (modified) clang/lib/Serialization/ASTWriter.cpp (+20-18) - (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+13-4) - (added) clang/test/Modules/no-transitive-change-namespace.cppm (+44) - (modified) clang/test/Modules/no-transitive-decl-change-4.cppm (+1-1) ``````````diff diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h index 1c4025f7482d9..867d0c96abca7 100644 --- a/clang/include/clang/Serialization/ASTWriter.h +++ b/clang/include/clang/Serialization/ASTWriter.h @@ -928,8 +928,6 @@ class ASTWriter : public ASTDeserializationListener, void handleVTable(CXXRecordDecl *RD); - void addTouchedModuleFile(serialization::ModuleFile *); - private: // ASTDeserializationListener implementation void ReaderInitialized(ASTReader *Reader) override; diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 3e10bbfedfe65..7e65c3940af5f 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -4084,10 +4084,6 @@ void ASTWriter::handleVTable(CXXRecordDecl *RD) { PendingEmittingVTables.push_back(RD); } -void ASTWriter::addTouchedModuleFile(serialization::ModuleFile *MF) { - TouchedModuleFiles.insert(MF); -} - //===----------------------------------------------------------------------===// // DeclContext's Name Lookup Table Serialization //===----------------------------------------------------------------------===// @@ -4132,7 +4128,6 @@ class ASTDeclContextNameLookupTraitBase { "have reference to loaded module file but no chain?"); using namespace llvm::support; - Writer.addTouchedModuleFile(F); endian::write<uint32_t>(Out, Writer.getChain()->getModuleFileID(F), llvm::endianness::little); } @@ -4330,6 +4325,14 @@ static bool isTULocalInNamedModules(NamedDecl *D) { return D->getLinkageInternal() == Linkage::Internal; } +static NamedDecl *getLocalRedecl(NamedDecl *D) { + for (auto *RD : D->redecls()) + if (!RD->isFromASTFile()) + return cast<NamedDecl>(RD); + + return D; +} + class ASTDeclContextNameLookupTrait : public ASTDeclContextNameTrivialLookupTrait { public: @@ -4402,6 +4405,13 @@ class ASTDeclContextNameLookupTrait auto AddDecl = [this](NamedDecl *D) { NamedDecl *DeclForLocalLookup = getDeclForLocalLookup(Writer.getLangOpts(), D); + + // Namespace may have many redeclarations in many TU. + // Also, these namespaces are symmetric. So that we try to use + // the local redecl if any. + if (isa<NamespaceDecl>(DeclForLocalLookup) && + DeclForLocalLookup->isFromASTFile()) + DeclForLocalLookup = getLocalRedecl(DeclForLocalLookup); if (Writer.getDoneWritingDeclsAndTypes() && !Writer.wasDeclEmitted(DeclForLocalLookup)) @@ -4524,7 +4534,6 @@ class LazySpecializationInfoLookupTrait { "have reference to loaded module file but no chain?"); using namespace llvm::support; - Writer.addTouchedModuleFile(F); endian::write<uint32_t>(Out, Writer.getChain()->getModuleFileID(F), llvm::endianness::little); } @@ -4623,7 +4632,7 @@ void ASTWriter::GenerateSpecializationInfoLookupTable( Generator.insert(HashValue, Trait.getData(Specs, ExisitingSpecs), Trait); } - Generator.emit(LookupTable, Trait, Lookups ? &Lookups->Table : nullptr); + Generator.emit(LookupTable, Trait, nullptr); } uint64_t ASTWriter::WriteSpecializationInfoLookupTable( @@ -4817,10 +4826,8 @@ void ASTWriter::GenerateNameLookupTable( Generator.insert(ConversionDecls.front()->getDeclName(), Trait.getData(ConversionDecls), Trait); - // Create the on-disk hash table. Also emit the existing imported and - // merged table if there is one. - auto *Lookups = Chain ? Chain->getLoadedLookupTables(DC) : nullptr; - Generator.emit(LookupTable, Trait, Lookups ? &Lookups->Table : nullptr); + // Create the on-disk hash table. + Generator.emit(LookupTable, Trait, nullptr); const auto &ModuleLocalDecls = Trait.getModuleLocalDecls(); if (!ModuleLocalDecls.empty()) { @@ -4836,11 +4843,8 @@ void ASTWriter::GenerateNameLookupTable( ModuleLocalTrait); } - auto *ModuleLocalLookups = - Chain ? Chain->getModuleLocalLookupTables(DC) : nullptr; ModuleLocalLookupGenerator.emit( - ModuleLocalLookupTable, ModuleLocalTrait, - ModuleLocalLookups ? &ModuleLocalLookups->Table : nullptr); + ModuleLocalLookupTable, ModuleLocalTrait, nullptr); } const auto &TULocalDecls = Trait.getTULocalDecls(); @@ -4856,9 +4860,7 @@ void ASTWriter::GenerateNameLookupTable( TULookupGenerator.insert(Key, TULocalTrait.getData(IDs), TULocalTrait); } - auto *TULocalLookups = Chain ? Chain->getTULocalLookupTables(DC) : nullptr; - TULookupGenerator.emit(TULookupTable, TULocalTrait, - TULocalLookups ? &TULocalLookups->Table : nullptr); + TULookupGenerator.emit(TULookupTable, TULocalTrait, nullptr); } } diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index 7646d5d5efe00..e39832f5c8e5c 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -2234,8 +2234,15 @@ void ASTDeclWriter::VisitRedeclarable(Redeclarable<T> *D) { // redecl chain. unsigned I = Record.size(); Record.push_back(0); - if (Writer.Chain) - AddFirstDeclFromEachModule(DAsT, /*IncludeLocal*/false); + if (Writer.Chain) { + // Namespace can have many redeclaration in many TU. + // We try to avoid touching every redeclaration to try to + // reduce the dependencies as much as possible. + if (!isa<NamespaceDecl>(DAsT)) + AddFirstDeclFromEachModule(DAsT, /*IncludeLocal*/false); + else + Record.AddDeclRef(nullptr); + } // This is the number of imported first declarations + 1. Record[I] = Record.size() - I; @@ -2265,8 +2272,10 @@ void ASTDeclWriter::VisitRedeclarable(Redeclarable<T> *D) { // // FIXME: This is not correct; when we reach an imported declaration we // won't emit its previous declaration. - (void)Writer.GetDeclRef(D->getPreviousDecl()); - (void)Writer.GetDeclRef(MostRecent); + if (D->getPreviousDecl() && !D->getPreviousDecl()->isFromASTFile()) + (void)Writer.GetDeclRef(D->getPreviousDecl()); + if (!MostRecent->isFromASTFile()) + (void)Writer.GetDeclRef(MostRecent); } else { // We use the sentinel value 0 to indicate an only declaration. Record.push_back(0); diff --git a/clang/test/Modules/no-transitive-change-namespace.cppm b/clang/test/Modules/no-transitive-change-namespace.cppm new file mode 100644 index 0000000000000..f67dd8533a6df --- /dev/null +++ b/clang/test/Modules/no-transitive-change-namespace.cppm @@ -0,0 +1,44 @@ +// Test that adding a new decl in a module which originally only contained a namespace +// won't break the dependency. +// +// RUN: rm -rf %t +// RUN: split-file %s %t +// +// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/Root.cppm -o %t/Root.pcm +// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/N.cppm -o %t/N.pcm \ +// RUN: -fmodule-file=Root=%t/Root.pcm +// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/N.v1.cppm -o %t/N.v1.pcm \ +// RUN: -fmodule-file=Root=%t/Root.pcm +// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/M.cppm -o %t/M.pcm \ +// RUN: -fmodule-file=N=%t/N.pcm -fmodule-file=Root=%t/Root.pcm +// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/M.cppm -o %t/M.v1.pcm \ +// RUN: -fmodule-file=N=%t/N.v1.pcm -fmodule-file=Root=%t/Root.pcm +// +// RUN: diff %t/M.pcm %t/M.v1.pcm &> /dev/null + +//--- Root.cppm +export module Root; +export namespace N { + +} + +//--- N.cppm +export module N; +import Root; +export namespace N { + +} + +//--- N.v1.cppm +export module N; +import Root; +export namespace N { +struct NN {}; +} + +//--- M.cppm +export module M; +import N; +export namespace N { +struct MM {}; +} diff --git a/clang/test/Modules/no-transitive-decl-change-4.cppm b/clang/test/Modules/no-transitive-decl-change-4.cppm index 944878be8df63..bb7e47f32669e 100644 --- a/clang/test/Modules/no-transitive-decl-change-4.cppm +++ b/clang/test/Modules/no-transitive-decl-change-4.cppm @@ -27,7 +27,7 @@ // RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/B.cppm -o %t/B.v1.pcm \ // RUN: -fprebuilt-module-path=%t -fmodule-file=AWrapper=%t/AWrapper.v1.pcm -fmodule-file=A=%t/A.v1.pcm // -// RUN: not diff %t/B.pcm %t/B.v1.pcm &> /dev/null +// RUN: diff %t/B.pcm %t/B.v1.pcm &> /dev/null //--- T.cppm export module T; `````````` </details> https://github.com/llvm/llvm-project/pull/179178 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
