[clang] [ASTWriter] Do not allocate source location space for module maps used only for textual headers (PR #116374)
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 15 03:27:57 PST 2024
https://github.com/ilya-biryukov created https://github.com/llvm/llvm-project/pull/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 for the 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.
>From ff328f256824e30b2c3c8db184a0fcafaef32637 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov <ibiryukov at google.com>
Date: Fri, 15 Nov 2024 11:18:21 +0100
Subject: [PATCH] [ASTWriter] Do not allocate source location space for module
maps used only for textual headers
This is a follow up to #112015 and it reduces the 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.
We do need the InputFile entry for the in the serialized AST because
clang-scan-deps relies on it. The previous patch introduced a mechanism
to do exactly that.
---
clang/lib/Serialization/ASTWriter.cpp | 46 +++++---
...-affecting-module-map-repeated-textual.cpp | 104 ++++++++++++++++++
2 files changed, 136 insertions(+), 14 deletions(-)
create mode 100644 clang/test/Modules/prune-non-affecting-module-map-repeated-textual.cpp
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 88b3e649a5d466..a0d0e03178296d 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -184,14 +184,29 @@ 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 {
+ ARTextualHeader = 0,
+ ARImportOrTextualHeader = 1,
+ };
+ auto AssignMostImportant = [](AffectedReason &L, AffectedReason R) {
+ L = std::max(L, R);
+ };
+ 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 of 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 +217,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 +231,7 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
// Handle all the affecting modules referenced from the root module.
- CollectModuleMapsForHierarchy(RootModule);
+ CollectModuleMapsForHierarchy(RootModule, ARImportOrTextualHeader);
std::queue<const Module *> Q;
Q.push(RootModule);
@@ -225,9 +240,9 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
Q.pop();
for (const Module *ImportedModule : CurrentModule->Imports)
- CollectModuleMapsForHierarchy(ImportedModule);
+ CollectModuleMapsForHierarchy(ImportedModule, ARImportOrTextualHeader);
for (const Module *UndeclaredModule : CurrentModule->UndeclaredUses)
- CollectModuleMapsForHierarchy(UndeclaredModule);
+ CollectModuleMapsForHierarchy(UndeclaredModule, ARImportOrTextualHeader);
for (auto *M : CurrentModule->submodules())
Q.push(M);
@@ -256,7 +271,7 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
for (const auto &KH : HS.findResolvedModulesForHeader(*File))
if (const Module *M = KH.getModule())
- CollectModuleMapsForHierarchy(M);
+ CollectModuleMapsForHierarchy(M, ARTextualHeader);
}
// FIXME: This algorithm is not correct for module map hierarchies where
@@ -278,13 +293,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 == ARImportOrTextualHeader)
+ 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}}
+
+// different 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