[clang][modules] Only compute affecting module maps with implicit search

[jansvoboda11]

When writing out a PCM, we compute the set of module maps that did affect the compilation and we strip the rest to make the output independent of them. The most common way to read a module map that is not affecting is with implicit module map search. The other option is to pass a bunch of unnecessary -fmodule-map-file=<path> arguments on the command-line, in which case the client should probably not give those to Clang anyway.

This makes serialization of explicit modules faster, mostly due to reduced file system traffic.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Jan Svoboda (jansvoboda11)

Changes

When writing out a PCM, we compute the set of module maps that did affect the compilation and we strip the rest to make the output independent of them. The most common way to read a module map that is not affecting is with implicit module map search. The other option is to pass a bunch of unnecessary -fmodule-map-file=<path> arguments on the command-line, in which case the client should probably not give those to Clang anyway.

This makes serialization of explicit modules faster, mostly due to reduced file system traffic.

---

Full diff: https://github.com/llvm/llvm-project/pull/87849.diff

1 Files Affected:

• (modified) clang/lib/Serialization/ASTWriter.cpp (+17-4)

diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index ba6a8a5e16e4e7..e1e22017595518 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -161,8 +161,13 @@ static TypeCode getTypeCodeForTypeClass(Type::TypeClass id) {
 
 namespace {
 
-std::set<const FileEntry *> GetAffectingModuleMaps(const Preprocessor &PP,
-                                                   Module *RootModule) {
+std::optional<std::set<const FileEntry *>>
+GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
+  // Without implicit module map search, there's no good reason to know about
+  // any module maps that are not affecting.
+  if (!PP.getHeaderSearchInfo().getHeaderSearchOpts().ImplicitModuleMaps)
+    return std::nullopt;
+
   SmallVector<const Module *> ModulesToProcess{RootModule};
 
   const HeaderSearch &HS = PP.getHeaderSearchInfo();
@@ -4733,8 +4738,16 @@ void ASTWriter::computeNonAffectingInputFiles() {
     if (!Cache->OrigEntry)
       continue;
 
-    if (!isModuleMap(File.getFileCharacteristic()) ||
-        llvm::is_contained(AffectingModuleMaps, *Cache->OrigEntry))
+    // Don't prune anything other than module maps.
+    if (!isModuleMap(File.getFileCharacteristic()))
+      continue;
+
+    // Don't prune module maps if all are guaranteed to be affecting.
+    if (!AffectingModuleMaps)
+      continue;
+
+    // Don't prune module maps that are affecting.
+    if (llvm::is_contained(*AffectingModuleMaps, *Cache->OrigEntry))
       continue;
 
     IsSLocAffecting[I] = false;

[jansvoboda11]

CC @ilyakuteev

[zygoloid]

I don't think this is correct -- we can know about non-affecting module map files because they were specified on the command line using -fmodule-map-file= and either weren't actually used or (as happens in our case) we got the same information from a PCM file and so didn't need the module map file.

I think this check is a regression; can it be removed?

[zygoloid]

Hm, I see that adding this check was the whole point of the patch. I don't think this works -- it's not feasible for the build system to know whether a module map file would affect a compilation or not.

[jansvoboda11]

I wasn't aware that you're passing unused module map files to the compiler. In that case I can introduce a separate flag to control this functionality and only disable the computation of affecting module maps for our flavor of explicit modules where this is a no-op. Sounds good?

[zygoloid]

Yes, that sounds great, thank you. In our case, it's not that the module map files are unused, it's that we specify the same -fmodule-map-file= flag to all of our compilations, and so we usually get the information from a .pcm file rather than from the module map file, and it turns out that pruning out the repeated module maps make a rather large difference to our source location address space usage :)

All that said, we have some other ideas for how to address the problems we're seeing with source location address space usage. Maybe we can get to a point where we're not relying on this at all -- I think this patch is probably the right behavior, even though I think it's contributing to a regression for us, so I think this new behavior should be the default, and we can clean up the flag once we don't need it any more.