[clang] f1d81db - [ASTWriter] Do not allocate source location space for module maps used only for textual headers (#116374)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 5 06:08:41 PST 2024
Author: Ilya Biryukov
Date: 2024-12-05T15:08:38+01:00
New Revision: f1d81dbd0573502134270495aa43e3ffb3193ab0
URL: https://github.com/llvm/llvm-project/commit/f1d81dbd0573502134270495aa43e3ffb3193ab0
DIFF: https://github.com/llvm/llvm-project/commit/f1d81dbd0573502134270495aa43e3ffb3193ab0.diff
LOG: [ASTWriter] Do not allocate source location space for module maps used only for textual headers (#116374)
This is a follow up to #112015 and it reduces the unnecessary
duplication of source locations further.
We do not need to allocate source location space in the serialized PCMs
for module maps used only to find textual headers. Those module maps are
never referenced from anywhere in the serialized ASTs and are re-read in
other compilations.
This change should not affect correctness of Clang compilations or
clang-scan-deps in any way.
We do need the InputFile entry in the serialized AST because
clang-scan-deps relies on it. The previous patch introduced a mechanism
to do exactly that.
We have found that to finally remove any duplication of module maps we
use internally in our build system.
Added:
clang/test/Modules/prune-non-affecting-module-map-repeated-textual.cpp
Modified:
clang/lib/Serialization/ASTWriter.cpp
Removed:
################################################################################
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index a52d59c61c4ce6..f8158a654c45aa 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -184,14 +184,30 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
const SourceManager &SM = PP.getSourceManager();
const ModuleMap &MM = HS.getModuleMap();
- llvm::DenseSet<FileID> ModuleMaps;
-
- llvm::DenseSet<const Module *> ProcessedModules;
- auto CollectModuleMapsForHierarchy = [&](const Module *M) {
+ // Module maps used only by textual headers are special. Their FileID is
+ // non-affecting, but their FileEntry is (i.e. must be written as InputFile).
+ enum AffectedReason : bool {
+ AR_TextualHeader = 0,
+ AR_ImportOrTextualHeader = 1,
+ };
+ auto AssignMostImportant = [](AffectedReason &LHS, AffectedReason RHS) {
+ LHS = std::max(LHS, RHS);
+ };
+ llvm::DenseMap<FileID, AffectedReason> ModuleMaps;
+ llvm::DenseMap<const Module *, AffectedReason> ProcessedModules;
+ auto CollectModuleMapsForHierarchy = [&](const Module *M,
+ AffectedReason Reason) {
M = M->getTopLevelModule();
- if (!ProcessedModules.insert(M).second)
+ // We need to process the header either when it was not present or when we
+ // previously flagged module map as textual headers and now we found a
+ // proper import.
+ if (auto [It, Inserted] = ProcessedModules.insert({M, Reason});
+ !Inserted && Reason <= It->second) {
return;
+ } else {
+ It->second = Reason;
+ }
std::queue<const Module *> Q;
Q.push(M);
@@ -202,12 +218,12 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
// The containing module map is affecting, because it's being pointed
// into by Module::DefinitionLoc.
if (auto F = MM.getContainingModuleMapFileID(Mod); F.isValid())
- ModuleMaps.insert(F);
+ AssignMostImportant(ModuleMaps[F], Reason);
// For inferred modules, the module map that allowed inferring is not
// related to the virtual containing module map file. It did affect the
// compilation, though.
if (auto UniqF = MM.getModuleMapFileIDForUniquing(Mod); UniqF.isValid())
- ModuleMaps.insert(UniqF);
+ AssignMostImportant(ModuleMaps[UniqF], Reason);
for (auto *SubM : Mod->submodules())
Q.push(SubM);
@@ -216,7 +232,7 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
// Handle all the affecting modules referenced from the root module.
- CollectModuleMapsForHierarchy(RootModule);
+ CollectModuleMapsForHierarchy(RootModule, AR_ImportOrTextualHeader);
std::queue<const Module *> Q;
Q.push(RootModule);
@@ -225,9 +241,9 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
Q.pop();
for (const Module *ImportedModule : CurrentModule->Imports)
- CollectModuleMapsForHierarchy(ImportedModule);
+ CollectModuleMapsForHierarchy(ImportedModule, AR_ImportOrTextualHeader);
for (const Module *UndeclaredModule : CurrentModule->UndeclaredUses)
- CollectModuleMapsForHierarchy(UndeclaredModule);
+ CollectModuleMapsForHierarchy(UndeclaredModule, AR_ImportOrTextualHeader);
for (auto *M : CurrentModule->submodules())
Q.push(M);
@@ -256,7 +272,7 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
for (const auto &KH : HS.findResolvedModulesForHeader(*File))
if (const Module *M = KH.getModule())
- CollectModuleMapsForHierarchy(M);
+ CollectModuleMapsForHierarchy(M, AR_TextualHeader);
}
// FIXME: This algorithm is not correct for module map hierarchies where
@@ -278,13 +294,16 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
// includes a module map defining a module that's not a submodule of X.
llvm::DenseSet<const FileEntry *> ModuleFileEntries;
- for (FileID MM : ModuleMaps) {
- if (auto *FE = SM.getFileEntryForID(MM))
+ llvm::DenseSet<FileID> ModuleFileIDs;
+ for (auto [FID, Reason] : ModuleMaps) {
+ if (Reason == AR_ImportOrTextualHeader)
+ ModuleFileIDs.insert(FID);
+ if (auto *FE = SM.getFileEntryForID(FID))
ModuleFileEntries.insert(FE);
}
AffectingModuleMaps R;
- R.DefinitionFileIDs = std::move(ModuleMaps);
+ R.DefinitionFileIDs = std::move(ModuleFileIDs);
R.DefinitionFiles = std::move(ModuleFileEntries);
return std::move(R);
}
diff --git a/clang/test/Modules/prune-non-affecting-module-map-repeated-textual.cpp b/clang/test/Modules/prune-non-affecting-module-map-repeated-textual.cpp
new file mode 100644
index 00000000000000..30914056c55fa2
--- /dev/null
+++ b/clang/test/Modules/prune-non-affecting-module-map-repeated-textual.cpp
@@ -0,0 +1,104 @@
+// Same as prune-non-affecting-module-map-repeated.cpp, but check that textual-only
+// inclusions do not cause duplication of the module map files they are defined in.
+
+// RUN: rm -rf %t && mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -xc++ -fmodules -fno-implicit-modules \
+// RUN: -fmodule-map-file=%t/base.map -fmodule-map-file=%t/mixed.map\
+// RUN: -fmodule-map-file=%t/mod1.map \
+// RUN: -fmodule-name=mod1 -emit-module %t/mod1.map -o %t/mod1.pcm
+// RUN: %clang_cc1 -xc++ -fmodules -fno-implicit-modules \
+// RUN: -fmodule-map-file=%t/mixed.map\
+// RUN: -fmodule-name=mixed -emit-module %t/mixed.map -o %t/mixed.pcm
+// RUN: %clang_cc1 -xc++ -fmodules -fno-implicit-modules \
+// RUN: -fmodule-map-file=%t/base.map -fmodule-map-file=%t/mod1.map -fmodule-map-file=%t/mod2.map \
+// RUN: -fmodule-file=%t/mod1.pcm -fmodule-file=%t/mixed.pcm \
+// RUN: -fmodule-name=mod2 -emit-module %t/mod2.map -o %t/mod2.pcm
+// RUN: %clang_cc1 -xc++ -fmodules -fno-implicit-modules \
+// RUN: -fmodule-map-file=%t/base.map -fmodule-map-file=%t/mod2.map -fmodule-map-file=%t/mod3.map \
+// RUN: -fmodule-file=%t/mod2.pcm -fmodule-file=%t/mixed.pcm \
+// RUN: -fmodule-name=mod3 -emit-module %t/mod3.map -o %t/mod3.pcm
+// RUN: %clang_cc1 -xc++ -fmodules -fno-implicit-modules \
+// RUN: -fmodule-map-file=%t/base.map -fmodule-map-file=%t/mod3.map -fmodule-map-file=%t/mod4.map \
+// RUN: -fmodule-file=%t/mod3.pcm \
+// RUN: -fmodule-name=mod4 -emit-module %t/mod4.map -o %t/mod4.pcm
+// RUN: %clang_cc1 -xc++ -fmodules -fno-implicit-modules -fmodule-map-file=%t/base.map -fmodule-map-file=%t/mod4.map -fmodule-file=%t/mod4.pcm -fsyntax-only -verify %t/check_slocs.cc
+
+//--- base.map
+module base { textual header "vector.h" }
+//--- mixed.map
+module mixed { textual header "mixed_text.h" header "mixed_mod.h"}
+//--- mod1.map
+module mod1 { header "mod1.h" }
+//--- mod2.map
+module mod2 { header "mod2.h" }
+//--- mod3.map
+module mod3 { header "mod3.h" }
+//--- mod4.map
+module mod4 { header "mod4.h" }
+//--- check_slocs.cc
+#include "mod4.h"
+#include "vector.h"
+#pragma clang __debug sloc_usage // expected-remark {{source manager location address space usage}}
+// expected-note@* {{% of available space}}
+
+// base.map must only be present once, despite being used in each module.
+// Because its location in every module compile should be non-affecting.
+
+// expected-note at base.map:1 {{file entered 1 time}}
+
+//
diff erent modules use either only textual header from mixed.map or both textual and modular
+// headers. Either combination must lead to only 1 use at the end, because the module is ultimately
+// in the import chain and any textual uses should not change that.
+
+// expected-note at mixed.map:1 {{file entered 1 time}}
+
+// expected-note@* + {{file entered}}
+
+
+//--- vector.h
+#ifndef VECTOR_H
+#define VECTOR_H
+#endif
+
+//--- mixed_text.h
+#ifndef MIXED_TEXT_H
+#define MIXED_TEXT_H
+#endif
+//--- mixed_mod.h
+#ifndef MIXED_MOD_H
+#define MIXED_MOD_H
+#endif
+
+//--- mod1.h
+#ifndef MOD1
+#define MOD1
+#include "vector.h"
+#include "mixed_text.h"
+int mod1();
+#endif
+//--- mod2.h
+#ifndef MOD2
+#define MOD2
+#include "vector.h"
+#include "mod1.h"
+#include "mixed_mod.h"
+int mod2();
+#endif
+//--- mod3.h
+#ifndef MOD3
+#define MOD3
+#include "vector.h"
+#include "mod2.h"
+#include "mixed_text.h"
+#include "mixed_mod.h"
+int mod3();
+#endif
+//--- mod4.h
+#ifndef MOD4
+#define MOD4
+#include "vector.h"
+#include "mod3.h"
+int mod4();
+#endif
More information about the cfe-commits
mailing list