llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Michael Park (mpark) <details> <summary>Changes</summary> This reverts commit 1928c1ea9b57e9c44325d436bc7bb2f4585031f3. --- Full diff: https://github.com/llvm/llvm-project/pull/174783.diff 4 Files Affected: - (modified) clang/lib/Serialization/ASTReader.cpp (+11-47) - (modified) clang/lib/Serialization/ASTWriter.cpp (+5-23) - (modified) clang/unittests/Serialization/CMakeLists.txt (-1) - (removed) clang/unittests/Serialization/NamespaceLookupTest.cpp (-247) ``````````diff diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index c2426e88158b9..66cf484bb5cb6 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -555,25 +555,7 @@ namespace { using MacroDefinitionsMap = llvm::StringMap<std::pair<StringRef, bool /*IsUndef*/>>; - -class DeclsSet { - SmallVector<NamedDecl *, 64> Decls; - llvm::SmallPtrSet<NamedDecl *, 8> Found; - -public: - operator ArrayRef<NamedDecl *>() const { return Decls; } - - bool empty() const { return Decls.empty(); } - - bool insert(NamedDecl *ND) { - auto [_, Inserted] = Found.insert(ND); - if (Inserted) - Decls.push_back(ND); - return Inserted; - } -}; - -using DeclsMap = llvm::DenseMap<DeclarationName, DeclsSet>; +using DeclsMap = llvm::DenseMap<DeclarationName, SmallVector<NamedDecl *, 8>>; } // namespace @@ -8752,23 +8734,14 @@ bool ASTReader::FindExternalVisibleDeclsByName(const DeclContext *DC, return false; // Load the list of declarations. - DeclsSet DS; + SmallVector<NamedDecl *, 64> Decls; + llvm::SmallPtrSet<NamedDecl *, 8> Found; auto Find = [&, this](auto &&Table, auto &&Key) { for (GlobalDeclID ID : Table.find(Key)) { NamedDecl *ND = cast<NamedDecl>(GetDecl(ID)); - if (ND->getDeclName() != Name) - continue; - // Special case for namespaces: There can be a lot of redeclarations of - // some namespaces, and we import a "key declaration" per imported module. - // Since all declarations of a namespace are essentially interchangeable, - // we can optimize namespace look-up by only storing the key declaration - // of the current TU, rather than storing N key declarations where N is - // the # of imported modules that declare that namespace. - // TODO: Try to generalize this optimization to other redeclarable decls. - if (isa<NamespaceDecl>(ND)) - ND = cast<NamedDecl>(getKeyDeclaration(ND)); - DS.insert(ND); + if (ND->getDeclName() == Name && Found.insert(ND).second) + Decls.push_back(ND); } }; @@ -8803,8 +8776,8 @@ bool ASTReader::FindExternalVisibleDeclsByName(const DeclContext *DC, Find(It->second.Table, Name); } - SetExternalVisibleDeclsForName(DC, Name, DS); - return !DS.empty(); + SetExternalVisibleDeclsForName(DC, Name, Decls); + return !Decls.empty(); } void ASTReader::completeVisibleDeclsMap(const DeclContext *DC) { @@ -8822,16 +8795,7 @@ void ASTReader::completeVisibleDeclsMap(const DeclContext *DC) { for (GlobalDeclID ID : It->second.Table.findAll()) { NamedDecl *ND = cast<NamedDecl>(GetDecl(ID)); - // Special case for namespaces: There can be a lot of redeclarations of - // some namespaces, and we import a "key declaration" per imported module. - // Since all declarations of a namespace are essentially interchangeable, - // we can optimize namespace look-up by only storing the key declaration - // of the current TU, rather than storing N key declarations where N is - // the # of imported modules that declare that namespace. - // TODO: Try to generalize this optimization to other redeclarable decls. - if (isa<NamespaceDecl>(ND)) - ND = cast<NamedDecl>(getKeyDeclaration(ND)); - Decls[ND->getDeclName()].insert(ND); + Decls[ND->getDeclName()].push_back(ND); } // FIXME: Why a PCH test is failing if we remove the iterator after findAll? @@ -8841,9 +8805,9 @@ void ASTReader::completeVisibleDeclsMap(const DeclContext *DC) { findAll(ModuleLocalLookups, NumModuleLocalVisibleDeclContexts); findAll(TULocalLookups, NumTULocalVisibleDeclContexts); - for (auto &[Name, DS] : Decls) - SetExternalVisibleDeclsForName(DC, Name, DS); - + for (DeclsMap::iterator I = Decls.begin(), E = Decls.end(); I != E; ++I) { + SetExternalVisibleDeclsForName(DC, I->first, I->second); + } const_cast<DeclContext *>(DC)->setHasExternalVisibleStorage(false); } diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index f9176b7e68f73..39104da10d0b7 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -4397,20 +4397,20 @@ class ASTDeclContextNameLookupTrait template <typename Coll> data_type getData(const Coll &Decls) { unsigned Start = DeclIDs.size(); - auto AddDecl = [this](NamedDecl *D) { + for (NamedDecl *D : Decls) { NamedDecl *DeclForLocalLookup = getDeclForLocalLookup(Writer.getLangOpts(), D); if (Writer.getDoneWritingDeclsAndTypes() && !Writer.wasDeclEmitted(DeclForLocalLookup)) - return; + continue; // Try to avoid writing internal decls to reduced BMI. // See comments in ASTWriter::WriteDeclContextLexicalBlock for details. if (Writer.isGeneratingReducedBMI() && !DeclForLocalLookup->isFromExplicitGlobalModule() && IsInternalDeclFromFileContext(DeclForLocalLookup)) - return; + continue; auto ID = Writer.GetDeclRef(DeclForLocalLookup); @@ -4424,7 +4424,7 @@ class ASTDeclContextNameLookupTrait ModuleLocalDeclsMap.insert({Key, DeclIDsTy{ID}}); else Iter->second.push_back(ID); - return; + continue; } break; case LookupVisibility::TULocal: { @@ -4433,7 +4433,7 @@ class ASTDeclContextNameLookupTrait TULocalDeclsMap.insert({D->getDeclName(), DeclIDsTy{ID}}); else Iter->second.push_back(ID); - return; + continue; } case LookupVisibility::GenerallyVisibile: // Generally visible decls go into the general lookup table. @@ -4441,24 +4441,6 @@ class ASTDeclContextNameLookupTrait } DeclIDs.push_back(ID); - }; - ASTReader *Chain = Writer.getChain(); - for (NamedDecl *D : Decls) { - if (Chain && isa<NamespaceDecl>(D) && D->isFromASTFile() && - D == Chain->getKeyDeclaration(D)) { - // In ASTReader, we stored only the key declaration of a namespace decl - // for this TU rather than storing all of the key declarations from each - // imported module. If we have an external namespace decl, this is that - // key declaration and we need to re-expand it to write out all of the - // key declarations from each imported module again. - // - // See comment 'ASTReader::FindExternalVisibleDeclsByName' for details. - Chain->forEachImportedKeyDecl(D, [&AddDecl](const Decl *D) { - AddDecl(cast<NamedDecl>(const_cast<Decl *>(D))); - }); - } else { - AddDecl(D); - } } return std::make_pair(Start, DeclIDs.size()); } diff --git a/clang/unittests/Serialization/CMakeLists.txt b/clang/unittests/Serialization/CMakeLists.txt index a5cc1ed83af49..6782e6b4d7330 100644 --- a/clang/unittests/Serialization/CMakeLists.txt +++ b/clang/unittests/Serialization/CMakeLists.txt @@ -2,7 +2,6 @@ add_clang_unittest(SerializationTests ForceCheckFileInputTest.cpp InMemoryModuleCacheTest.cpp ModuleCacheTest.cpp - NamespaceLookupTest.cpp NoCommentsTest.cpp PreambleInNamedModulesTest.cpp LoadSpecLazilyTest.cpp diff --git a/clang/unittests/Serialization/NamespaceLookupTest.cpp b/clang/unittests/Serialization/NamespaceLookupTest.cpp deleted file mode 100644 index eefa4be9fbee5..0000000000000 --- a/clang/unittests/Serialization/NamespaceLookupTest.cpp +++ /dev/null @@ -1,247 +0,0 @@ -//== unittests/Serialization/NamespaceLookupOptimizationTest.cpp =======// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -#include "clang/Driver/CreateInvocationFromArgs.h" -#include "clang/Frontend/CompilerInstance.h" -#include "clang/Frontend/FrontendAction.h" -#include "clang/Frontend/FrontendActions.h" -#include "clang/Parse/ParseAST.h" -#include "clang/Serialization/ASTReader.h" -#include "clang/Tooling/Tooling.h" -#include "gtest/gtest.h" - -using namespace llvm; -using namespace clang; -using namespace clang::tooling; - -namespace { - -class NamespaceLookupTest : public ::testing::Test { - void SetUp() override { - ASSERT_FALSE( - sys::fs::createUniqueDirectory("namespace-lookup-test", TestDir)); - } - - void TearDown() override { sys::fs::remove_directories(TestDir); } - -public: - SmallString<256> TestDir; - - void addFile(StringRef Path, StringRef Contents) { - ASSERT_FALSE(sys::path::is_absolute(Path)); - - SmallString<256> AbsPath(TestDir); - sys::path::append(AbsPath, Path); - - ASSERT_FALSE( - sys::fs::create_directories(llvm::sys::path::parent_path(AbsPath))); - - std::error_code EC; - llvm::raw_fd_ostream OS(AbsPath, EC); - ASSERT_FALSE(EC); - OS << Contents; - } - - std::string GenerateModuleInterface(StringRef ModuleName, - StringRef Contents) { - std::string FileName = llvm::Twine(ModuleName + ".cppm").str(); - addFile(FileName, Contents); - - IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = - llvm::vfs::createPhysicalFileSystem(); - DiagnosticOptions DiagOpts; - IntrusiveRefCntPtr<DiagnosticsEngine> Diags = - CompilerInstance::createDiagnostics(*VFS, DiagOpts); - CreateInvocationOptions CIOpts; - CIOpts.Diags = Diags; - CIOpts.VFS = VFS; - - std::string CacheBMIPath = - llvm::Twine(TestDir + "/" + ModuleName + ".pcm").str(); - std::string PrebuiltModulePath = - "-fprebuilt-module-path=" + TestDir.str().str(); - const char *Args[] = {"clang++", - "-std=c++20", - "--precompile", - PrebuiltModulePath.c_str(), - "-working-directory", - TestDir.c_str(), - "-I", - TestDir.c_str(), - FileName.c_str(), - "-o", - CacheBMIPath.c_str()}; - std::shared_ptr<CompilerInvocation> Invocation = - createInvocation(Args, CIOpts); - EXPECT_TRUE(Invocation); - - CompilerInstance Instance(std::move(Invocation)); - Instance.setDiagnostics(Diags); - Instance.getFrontendOpts().OutputFile = CacheBMIPath; - // Avoid memory leaks. - Instance.getFrontendOpts().DisableFree = false; - GenerateModuleInterfaceAction Action; - EXPECT_TRUE(Instance.ExecuteAction(Action)); - EXPECT_FALSE(Diags->hasErrorOccurred()); - - return CacheBMIPath; - } -}; - -struct NamespaceLookupResult { - int NumLocalNamespaces = 0; - int NumExternalNamespaces = 0; -}; - -class NamespaceLookupConsumer : public ASTConsumer { - NamespaceLookupResult &Result; - -public: - explicit NamespaceLookupConsumer(NamespaceLookupResult &Result) - : Result(Result) {} - - void HandleTranslationUnit(ASTContext &Context) override { - TranslationUnitDecl *TU = Context.getTranslationUnitDecl(); - ASSERT_TRUE(TU); - ASTReader *Chain = dyn_cast_or_null<ASTReader>(Context.getExternalSource()); - ASSERT_TRUE(Chain); - for (const Decl *D : - TU->lookup(DeclarationName(&Context.Idents.get("N")))) { - if (!isa<NamespaceDecl>(D)) - continue; - if (!D->isFromASTFile()) { - ++Result.NumLocalNamespaces; - } else { - ++Result.NumExternalNamespaces; - EXPECT_EQ(D, Chain->getKeyDeclaration(D)); - } - } - } -}; - -class NamespaceLookupAction : public ASTFrontendAction { - NamespaceLookupResult &Result; - -public: - explicit NamespaceLookupAction(NamespaceLookupResult &Result) - : Result(Result) {} - - std::unique_ptr<ASTConsumer> - CreateASTConsumer(CompilerInstance &CI, StringRef /*Unused*/) override { - return std::make_unique<NamespaceLookupConsumer>(Result); - } -}; - -TEST_F(NamespaceLookupTest, ExternalNamespacesOnly) { - GenerateModuleInterface("M1", R"cpp( -export module M1; -namespace N {} - )cpp"); - GenerateModuleInterface("M2", R"cpp( -export module M2; -namespace N {} - )cpp"); - GenerateModuleInterface("M3", R"cpp( -export module M3; -namespace N {} - )cpp"); - const char *test_file_contents = R"cpp( -import M1; -import M2; -import M3; - )cpp"; - std::string DepArg = "-fprebuilt-module-path=" + TestDir.str().str(); - NamespaceLookupResult Result; - EXPECT_TRUE(runToolOnCodeWithArgs( - std::make_unique<NamespaceLookupAction>(Result), test_file_contents, - { - "-std=c++20", - DepArg.c_str(), - "-I", - TestDir.c_str(), - }, - "main.cpp")); - - EXPECT_EQ(0, Result.NumLocalNamespaces); - EXPECT_EQ(1, Result.NumExternalNamespaces); -} - -TEST_F(NamespaceLookupTest, ExternalReplacedByLocal) { - GenerateModuleInterface("M1", R"cpp( -export module M1; -namespace N {} - )cpp"); - GenerateModuleInterface("M2", R"cpp( -export module M2; -namespace N {} - )cpp"); - GenerateModuleInterface("M3", R"cpp( -export module M3; -namespace N {} - )cpp"); - const char *test_file_contents = R"cpp( -import M1; -import M2; -import M3; - -namespace N {} - )cpp"; - std::string DepArg = "-fprebuilt-module-path=" + TestDir.str().str(); - NamespaceLookupResult Result; - EXPECT_TRUE(runToolOnCodeWithArgs( - std::make_unique<NamespaceLookupAction>(Result), test_file_contents, - { - "-std=c++20", - DepArg.c_str(), - "-I", - TestDir.c_str(), - }, - "main.cpp")); - - EXPECT_EQ(1, Result.NumLocalNamespaces); - EXPECT_EQ(0, Result.NumExternalNamespaces); -} - -TEST_F(NamespaceLookupTest, LocalAndExternalInterleaved) { - GenerateModuleInterface("M1", R"cpp( -export module M1; -namespace N {} - )cpp"); - GenerateModuleInterface("M2", R"cpp( -export module M2; -namespace N {} - )cpp"); - GenerateModuleInterface("M3", R"cpp( -export module M3; -namespace N {} - )cpp"); - const char *test_file_contents = R"cpp( -import M1; - -namespace N {} - -import M2; -import M3; - )cpp"; - std::string DepArg = "-fprebuilt-module-path=" + TestDir.str().str(); - NamespaceLookupResult Result; - EXPECT_TRUE(runToolOnCodeWithArgs( - std::make_unique<NamespaceLookupAction>(Result), test_file_contents, - { - "-std=c++20", - DepArg.c_str(), - "-I", - TestDir.c_str(), - }, - "main.cpp")); - - EXPECT_EQ(1, Result.NumLocalNamespaces); - EXPECT_EQ(1, Result.NumExternalNamespaces); -} - -} // namespace `````````` </details> https://github.com/llvm/llvm-project/pull/174783 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
