Conversation
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Michael Park (mpark) ChangesThis reverts commit 1928c1e. Full diff: https://github.com/llvm/llvm-project/pull/174783.diff 4 Files Affected:
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
|
…dules. (llvm#171769)" This reverts commit 1928c1e.
mpark
force-pushed
the
revert-modules-perf-namespaces
branch
from
4816fff to
620b9da
Compare
rupprecht
approved these changes
Jan 7, 2026
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/21164 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/29746 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/136/builds/6375 Here is the relevant piece of the build log for the reference |
Priyanshu3820
pushed a commit
to Priyanshu3820/llvm-project
that referenced
this pull request
Jan 18, 2026
…dules. (llvm#171769)" (llvm#174783) This reverts commit 1928c1e. We have at least one repro, but I won't be able to work on this until next week. Also with Clang 22 cut upcoming, we probably need to revert for now.
mpark
added a commit
to mpark/llvm-project
that referenced
this pull request
Jan 21, 2026
…odules. (llvm#171769)" (llvm#174783) This reverts commit c6e0e7d.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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 reverts commit 1928c1e.
We have at least one repro, but I won't be able to work on this until next week. Also with Clang 22 cut upcoming, we probably need to revert for now.