[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-28 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 closed 
https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-28 Thread Ben Langmuir via cfe-commits 

https://github.com/benlangmuir approved this pull request.

Thanks for explaining; LGTM

https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-25 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 edited 
https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-25 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 edited 
https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-25 Thread Jan Svoboda via cfe-commits 


@@ -71,6 +71,7 @@
 // CHECK-NEXT:   "context-hash": "{{.*}}",
 // CHECK-NEXT:   "file-deps": [
 // CHECK-NEXT: "[[PREFIX]]/first/module.modulemap",
+// CHECK-NEXT: "[[PREFIX]]/second/module.modulemap",

jansvoboda11 wrote:

This is a side-effect of the change in `CompilerInstance.cpp`. The implicit 
scan now uses the top-level module map to compile modules instead of the 
containing module map and that makes it included in file dependencies. This 
change is a bit unfortunate, since the explicit command line we generate 
actually only uses the containing module map, not the top-level one. Ideally, 
we'd either find a way to remove this file dependency or we'd make the explicit 
compile to also consume the top-level module map (in which case this file 
dependency would be real).

https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-25 Thread Jan Svoboda via cfe-commits 


@@ -1337,9 +1337,24 @@ static bool compileModule(CompilerInstance 
&ImportingInstance,
   // Get or create the module map that we'll use to build this module.
   ModuleMap &ModMap
 = ImportingInstance.getPreprocessor().getHeaderSearchInfo().getModuleMap();
+  SourceManager &SourceMgr = ImportingInstance.getSourceManager();
   bool Result;
-  if (OptionalFileEntryRef ModuleMapFile =
-  ModMap.getContainingModuleMapFile(Module)) {
+  if (FileID ModuleMapFID = ModMap.getContainingModuleMapFileID(Module);
+  ModuleMapFID.isValid()) {
+// We want to use the top-level module map. If we don't, the compiling

jansvoboda11 wrote:

Consider that `SourceManager::translateFile()` always prefers local 
`SLocEntries`. Now let's say that a module is defined in a module map that gets 
included via the `extern` directive from a top-level module map Clang find via 
header search. Before loading PCM for this module, the following code will 
resolve to the local `SLocEntry` (there are no loaded entries yet).

```
SourceManager::translateFile(
  SourceManager::getFileEntryRefForID( // 
ModuleMap::getContainingModuleMapFile(Module)
SourceManager::getFileID(  //
  Module::DefinitionLoc))) //
```

After compiling and loading the PCM for such module, `Module::DefinitionLoc` 
gets updated and now points into the loaded `SLocEntry`. Therefore, 
`ModuleMap::getContainingModuleMapFile(Module)` returns the loaded `SLocEntry`. 
Then, `SourceManager::translateFile()` maps that to the **local** `SLocEntry`, 
so we end up with a result identical to the previous one.

This patch gets rid of the unnecessary conversions and does this instead.

```
SourceManager::getFileID(Module::DefinitionLoc)))
```

This means that after loading the PCM for the module, this points into the 
table of loaded `SLocEntries`. The importer knows that the containing module 
map is not a top level module map. When it attempts to walk up the include 
chain to get the the top-level module map for this module (using 
`SourceManager::getIncludeLoc()`) it gets an invalid `SourceLocation`. This is 
because the PCM was not compiled from the top-level module map, its 
`SourceManager` never knew about the file, and therefore can't walk up.

This means that the true top-level module map never makes it into the set of 
affecting module maps we compute in `ASTWriter`. It's in turn left out from the 
`INPUT_FILE` block that only has the containing module map that's marked as 
not-top-level file. This ultimately breaks the dependency scanner. When 
generating `-fmodule-map-file=` arguments for the explicit compilation, it only 
uses files that made it into the `INPUT_FILE` block (are affecting) and are 
top-level (to avoid exploding the command line with transitive module maps). 
This then breaks the explicit compile, which doesn't have any (neither the 
top-level nor the containing module map) for the module in question.

So this piece of code is here to ensure implicit builds are always invoked with 
the top-level module map, not the containing one. This fixes the walk over the 
include chain of loaded `SLocEntries` and fixes 
`clang/test/ClangScanDeps/modules-extern-unrelated.m`.

https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-25 Thread Ben Langmuir via cfe-commits 


@@ -71,6 +71,7 @@
 // CHECK-NEXT:   "context-hash": "{{.*}}",
 // CHECK-NEXT:   "file-deps": [
 // CHECK-NEXT: "[[PREFIX]]/first/module.modulemap",
+// CHECK-NEXT: "[[PREFIX]]/second/module.modulemap",

benlangmuir wrote:

Why did this change?

https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-25 Thread Ben Langmuir via cfe-commits 


@@ -1337,9 +1337,24 @@ static bool compileModule(CompilerInstance 
&ImportingInstance,
   // Get or create the module map that we'll use to build this module.
   ModuleMap &ModMap
 = ImportingInstance.getPreprocessor().getHeaderSearchInfo().getModuleMap();
+  SourceManager &SourceMgr = ImportingInstance.getSourceManager();
   bool Result;
-  if (OptionalFileEntryRef ModuleMapFile =
-  ModMap.getContainingModuleMapFile(Module)) {
+  if (FileID ModuleMapFID = ModMap.getContainingModuleMapFileID(Module);
+  ModuleMapFID.isValid()) {
+// We want to use the top-level module map. If we don't, the compiling

benlangmuir wrote:

What is the connection between this change and the rest of the commit?

https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-25 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 updated 
https://github.com/llvm/llvm-project/pull/86216

>From 4dd48b40a034edf0b124ab08055a334ad7abd5ba Mon Sep 17 00:00:00 2001
From: Jan Svoboda 
Date: Thu, 21 Mar 2024 14:40:02 -0700
Subject: [PATCH 1/4] [clang][modules] Avoid calling expensive
 `SourceManager::translateFile()`

---
 clang/include/clang/Lex/ModuleMap.h   | 15 +++---
 clang/lib/Frontend/FrontendAction.cpp |  7 ++-
 clang/lib/Lex/ModuleMap.cpp   | 66 +++
 clang/lib/Serialization/ASTWriter.cpp | 23 +-
 4 files changed, 61 insertions(+), 50 deletions(-)

diff --git a/clang/include/clang/Lex/ModuleMap.h 
b/clang/include/clang/Lex/ModuleMap.h
index 867cb6eab42f2d..2e28ff6823cb2a 100644
--- a/clang/include/clang/Lex/ModuleMap.h
+++ b/clang/include/clang/Lex/ModuleMap.h
@@ -263,8 +263,8 @@ class ModuleMap {
 Attributes Attrs;
 
 /// If \c InferModules is non-zero, the module map file that allowed
-/// inferred modules.  Otherwise, nullopt.
-OptionalFileEntryRef ModuleMapFile;
+/// inferred modules.  Otherwise, invalid.
+FileID ModuleMapFID;
 
 /// The names of modules that cannot be inferred within this
 /// directory.
@@ -279,8 +279,7 @@ class ModuleMap {
 
   /// A mapping from an inferred module to the module map that allowed the
   /// inference.
-  // FIXME: Consider making the values non-optional.
-  llvm::DenseMap InferredModuleAllowedBy;
+  llvm::DenseMap InferredModuleAllowedBy;
 
   llvm::DenseMap AdditionalModMaps;
 
@@ -618,8 +617,9 @@ class ModuleMap {
   ///
   /// \param Module The module whose module map file will be returned, if 
known.
   ///
-  /// \returns The file entry for the module map file containing the given
-  /// module, or nullptr if the module definition was inferred.
+  /// \returns The FileID for the module map file containing the given module,
+  /// invalid if the module definition was inferred.
+  FileID getContainingModuleMapFileID(const Module *Module) const;
   OptionalFileEntryRef getContainingModuleMapFile(const Module *Module) const;
 
   /// Get the module map file that (along with the module name) uniquely
@@ -631,9 +631,10 @@ class ModuleMap {
   /// of inferred modules, returns the module map that allowed the inference
   /// (e.g. contained 'module *'). Otherwise, returns
   /// getContainingModuleMapFile().
+  FileID getModuleMapFileIDForUniquing(const Module *M) const;
   OptionalFileEntryRef getModuleMapFileForUniquing(const Module *M) const;
 
-  void setInferredModuleAllowedBy(Module *M, OptionalFileEntryRef ModMap);
+  void setInferredModuleAllowedBy(Module *M, FileID ModMapFID);
 
   /// Canonicalize \p Path in a manner suitable for a module map file. In
   /// particular, this canonicalizes the parent directory separately from the
diff --git a/clang/lib/Frontend/FrontendAction.cpp 
b/clang/lib/Frontend/FrontendAction.cpp
index b9fd9b8897b7e7..bc4c901325c7e1 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -535,8 +535,13 @@ static Module *prepareToBuildModule(CompilerInstance &CI,
 if (*OriginalModuleMap != CI.getSourceManager().getFileEntryRefForID(
   CI.getSourceManager().getMainFileID())) {
   M->IsInferred = true;
+  bool IsSystem = false; // TODO: Propagate the real thing here.
+  auto FileCharacter =
+  IsSystem ? SrcMgr::C_System_ModuleMap : SrcMgr::C_User_ModuleMap;
+  FileID OriginalModuleMapFID = CI.getSourceManager().getOrCreateFileID(
+  *OriginalModuleMap, FileCharacter);
   CI.getPreprocessor().getHeaderSearchInfo().getModuleMap()
-.setInferredModuleAllowedBy(M, *OriginalModuleMap);
+.setInferredModuleAllowedBy(M, OriginalModuleMapFID);
 }
   }
 
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 10c475f617d485..eed7eca2e73562 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -648,8 +648,7 @@ 
ModuleMap::findOrCreateModuleForHeaderInUmbrellaDir(FileEntryRef File) {
   UmbrellaModule = UmbrellaModule->Parent;
 
 if (UmbrellaModule->InferSubmodules) {
-  OptionalFileEntryRef UmbrellaModuleMap =
-  getModuleMapFileForUniquing(UmbrellaModule);
+  FileID UmbrellaModuleMap = getModuleMapFileIDForUniquing(UmbrellaModule);
 
   // Infer submodules for each of the directories we found between
   // the directory of the umbrella header and the directory where
@@ -1021,7 +1020,7 @@ Module *ModuleMap::inferFrameworkModule(DirectoryEntryRef 
FrameworkDir,
 
   // If the framework has a parent path from which we're allowed to infer
   // a framework module, do so.
-  OptionalFileEntryRef ModuleMapFile;
+  FileID ModuleMapFID;
   if (!Parent) {
 // Determine whether we're allowed to infer a module map.
 bool canInfer = false;
@@ -1060,7 +1059,7 @@ Module *ModuleMap::inferFrameworkModule(DirectoryEntryRef 
FrameworkDir,
   Attrs.IsExhaustive |= infer

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-25 Thread via cfe-commits 

github-actions[bot] wrote:



:white_check_mark: With the latest revision this PR passed the Python code 
formatter.

https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-25 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Jan Svoboda (jansvoboda11)


Changes

The `ASTWriter` algorithm for computing affecting module maps uses 
`SourceManager::translateFile()` to get a `FileID` from a `FileEntry`. This is 
slow (O(n)) since the function performs a linear walk over `SLocEntries` until 
it finds one with a matching `FileEntry`.

This patch removes this use of `SourceManager::translateFile()` by tracking 
`FileID` instead of `FileEntry` in couple of places in `ModuleMap`, giving 
`ASTWriter` the desired `FileID` directly. There are no changes required for 
clients that still want a `FileEntry` from `ModuleMap`: the existing APIs 
internally use `SourceManager` to perform the reverse `FileID` to `FileEntry` 
conversion in O(1).

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


6 Files Affected:

- (modified) clang/include/clang/Lex/ModuleMap.h (+8-7) 
- (modified) clang/lib/Frontend/CompilerInstance.cpp (+17-2) 
- (modified) clang/lib/Frontend/FrontendAction.cpp (+5-1) 
- (modified) clang/lib/Lex/ModuleMap.cpp (+36-30) 
- (modified) clang/lib/Serialization/ASTWriter.cpp (+8-9) 
- (modified) clang/test/ClangScanDeps/modules-extern-unrelated.m (+1) 


``diff
diff --git a/clang/include/clang/Lex/ModuleMap.h 
b/clang/include/clang/Lex/ModuleMap.h
index 867cb6eab42f2d..2e28ff6823cb2a 100644
--- a/clang/include/clang/Lex/ModuleMap.h
+++ b/clang/include/clang/Lex/ModuleMap.h
@@ -263,8 +263,8 @@ class ModuleMap {
 Attributes Attrs;
 
 /// If \c InferModules is non-zero, the module map file that allowed
-/// inferred modules.  Otherwise, nullopt.
-OptionalFileEntryRef ModuleMapFile;
+/// inferred modules.  Otherwise, invalid.
+FileID ModuleMapFID;
 
 /// The names of modules that cannot be inferred within this
 /// directory.
@@ -279,8 +279,7 @@ class ModuleMap {
 
   /// A mapping from an inferred module to the module map that allowed the
   /// inference.
-  // FIXME: Consider making the values non-optional.
-  llvm::DenseMap InferredModuleAllowedBy;
+  llvm::DenseMap InferredModuleAllowedBy;
 
   llvm::DenseMap AdditionalModMaps;
 
@@ -618,8 +617,9 @@ class ModuleMap {
   ///
   /// \param Module The module whose module map file will be returned, if 
known.
   ///
-  /// \returns The file entry for the module map file containing the given
-  /// module, or nullptr if the module definition was inferred.
+  /// \returns The FileID for the module map file containing the given module,
+  /// invalid if the module definition was inferred.
+  FileID getContainingModuleMapFileID(const Module *Module) const;
   OptionalFileEntryRef getContainingModuleMapFile(const Module *Module) const;
 
   /// Get the module map file that (along with the module name) uniquely
@@ -631,9 +631,10 @@ class ModuleMap {
   /// of inferred modules, returns the module map that allowed the inference
   /// (e.g. contained 'module *'). Otherwise, returns
   /// getContainingModuleMapFile().
+  FileID getModuleMapFileIDForUniquing(const Module *M) const;
   OptionalFileEntryRef getModuleMapFileForUniquing(const Module *M) const;
 
-  void setInferredModuleAllowedBy(Module *M, OptionalFileEntryRef ModMap);
+  void setInferredModuleAllowedBy(Module *M, FileID ModMapFID);
 
   /// Canonicalize \p Path in a manner suitable for a module map file. In
   /// particular, this canonicalizes the parent directory separately from the
diff --git a/clang/lib/Frontend/CompilerInstance.cpp 
b/clang/lib/Frontend/CompilerInstance.cpp
index 019f847ccbaad0..79ebb0ae0ee98f 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1337,9 +1337,24 @@ static bool compileModule(CompilerInstance 
&ImportingInstance,
   // Get or create the module map that we'll use to build this module.
   ModuleMap &ModMap
 = ImportingInstance.getPreprocessor().getHeaderSearchInfo().getModuleMap();
+  SourceManager &SourceMgr = ImportingInstance.getSourceManager();
   bool Result;
-  if (OptionalFileEntryRef ModuleMapFile =
-  ModMap.getContainingModuleMapFile(Module)) {
+  if (FileID ModuleMapFID = ModMap.getContainingModuleMapFileID(Module);
+  ModuleMapFID.isValid()) {
+// We want to use the top-level module map. If we don't, the compiling
+// instance may think the containing module map is a top-level one, while
+// the importing instance knows it's included from a parent module map via
+// the extern directive. This mismatch could bite us later.
+SourceLocation Loc = SourceMgr.getIncludeLoc(ModuleMapFID);
+while (Loc.isValid() && isModuleMap(SourceMgr.getFileCharacteristic(Loc))) 
{
+  ModuleMapFID = SourceMgr.getFileID(Loc);
+  Loc = SourceMgr.getIncludeLoc(ModuleMapFID);
+}
+
+OptionalFileEntryRef ModuleMapFile =
+SourceMgr.getFileEntryRefForID(ModuleMapFID);
+assert(ModuleMapFile && "Top-level module map with no FileID");
+
 // Canonicalize compilation to start with the pu

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-25 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 ready_for_review 
https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-25 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 updated 
https://github.com/llvm/llvm-project/pull/86216

>From 4dd48b40a034edf0b124ab08055a334ad7abd5ba Mon Sep 17 00:00:00 2001
From: Jan Svoboda 
Date: Thu, 21 Mar 2024 14:40:02 -0700
Subject: [PATCH 1/3] [clang][modules] Avoid calling expensive
 `SourceManager::translateFile()`

---
 clang/include/clang/Lex/ModuleMap.h   | 15 +++---
 clang/lib/Frontend/FrontendAction.cpp |  7 ++-
 clang/lib/Lex/ModuleMap.cpp   | 66 +++
 clang/lib/Serialization/ASTWriter.cpp | 23 +-
 4 files changed, 61 insertions(+), 50 deletions(-)

diff --git a/clang/include/clang/Lex/ModuleMap.h 
b/clang/include/clang/Lex/ModuleMap.h
index 867cb6eab42f2d..2e28ff6823cb2a 100644
--- a/clang/include/clang/Lex/ModuleMap.h
+++ b/clang/include/clang/Lex/ModuleMap.h
@@ -263,8 +263,8 @@ class ModuleMap {
 Attributes Attrs;
 
 /// If \c InferModules is non-zero, the module map file that allowed
-/// inferred modules.  Otherwise, nullopt.
-OptionalFileEntryRef ModuleMapFile;
+/// inferred modules.  Otherwise, invalid.
+FileID ModuleMapFID;
 
 /// The names of modules that cannot be inferred within this
 /// directory.
@@ -279,8 +279,7 @@ class ModuleMap {
 
   /// A mapping from an inferred module to the module map that allowed the
   /// inference.
-  // FIXME: Consider making the values non-optional.
-  llvm::DenseMap InferredModuleAllowedBy;
+  llvm::DenseMap InferredModuleAllowedBy;
 
   llvm::DenseMap AdditionalModMaps;
 
@@ -618,8 +617,9 @@ class ModuleMap {
   ///
   /// \param Module The module whose module map file will be returned, if 
known.
   ///
-  /// \returns The file entry for the module map file containing the given
-  /// module, or nullptr if the module definition was inferred.
+  /// \returns The FileID for the module map file containing the given module,
+  /// invalid if the module definition was inferred.
+  FileID getContainingModuleMapFileID(const Module *Module) const;
   OptionalFileEntryRef getContainingModuleMapFile(const Module *Module) const;
 
   /// Get the module map file that (along with the module name) uniquely
@@ -631,9 +631,10 @@ class ModuleMap {
   /// of inferred modules, returns the module map that allowed the inference
   /// (e.g. contained 'module *'). Otherwise, returns
   /// getContainingModuleMapFile().
+  FileID getModuleMapFileIDForUniquing(const Module *M) const;
   OptionalFileEntryRef getModuleMapFileForUniquing(const Module *M) const;
 
-  void setInferredModuleAllowedBy(Module *M, OptionalFileEntryRef ModMap);
+  void setInferredModuleAllowedBy(Module *M, FileID ModMapFID);
 
   /// Canonicalize \p Path in a manner suitable for a module map file. In
   /// particular, this canonicalizes the parent directory separately from the
diff --git a/clang/lib/Frontend/FrontendAction.cpp 
b/clang/lib/Frontend/FrontendAction.cpp
index b9fd9b8897b7e7..bc4c901325c7e1 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -535,8 +535,13 @@ static Module *prepareToBuildModule(CompilerInstance &CI,
 if (*OriginalModuleMap != CI.getSourceManager().getFileEntryRefForID(
   CI.getSourceManager().getMainFileID())) {
   M->IsInferred = true;
+  bool IsSystem = false; // TODO: Propagate the real thing here.
+  auto FileCharacter =
+  IsSystem ? SrcMgr::C_System_ModuleMap : SrcMgr::C_User_ModuleMap;
+  FileID OriginalModuleMapFID = CI.getSourceManager().getOrCreateFileID(
+  *OriginalModuleMap, FileCharacter);
   CI.getPreprocessor().getHeaderSearchInfo().getModuleMap()
-.setInferredModuleAllowedBy(M, *OriginalModuleMap);
+.setInferredModuleAllowedBy(M, OriginalModuleMapFID);
 }
   }
 
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 10c475f617d485..eed7eca2e73562 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -648,8 +648,7 @@ 
ModuleMap::findOrCreateModuleForHeaderInUmbrellaDir(FileEntryRef File) {
   UmbrellaModule = UmbrellaModule->Parent;
 
 if (UmbrellaModule->InferSubmodules) {
-  OptionalFileEntryRef UmbrellaModuleMap =
-  getModuleMapFileForUniquing(UmbrellaModule);
+  FileID UmbrellaModuleMap = getModuleMapFileIDForUniquing(UmbrellaModule);
 
   // Infer submodules for each of the directories we found between
   // the directory of the umbrella header and the directory where
@@ -1021,7 +1020,7 @@ Module *ModuleMap::inferFrameworkModule(DirectoryEntryRef 
FrameworkDir,
 
   // If the framework has a parent path from which we're allowed to infer
   // a framework module, do so.
-  OptionalFileEntryRef ModuleMapFile;
+  FileID ModuleMapFID;
   if (!Parent) {
 // Determine whether we're allowed to infer a module map.
 bool canInfer = false;
@@ -1060,7 +1059,7 @@ Module *ModuleMap::inferFrameworkModule(DirectoryEntryRef 
FrameworkDir,
   Attrs.IsExhaustive |= infer

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-21 Thread via cfe-commits 

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff 6b1cf0040059c407264d2609403c4fc090673167 
4dd48b40a034edf0b124ab08055a334ad7abd5ba -- clang/include/clang/Lex/ModuleMap.h 
clang/lib/Frontend/FrontendAction.cpp clang/lib/Lex/ModuleMap.cpp 
clang/lib/Serialization/ASTWriter.cpp
``





View the diff from clang-format here.


``diff
diff --git a/clang/lib/Frontend/FrontendAction.cpp 
b/clang/lib/Frontend/FrontendAction.cpp
index bc4c901325..5921a8c065 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -540,8 +540,10 @@ static Module *prepareToBuildModule(CompilerInstance &CI,
   IsSystem ? SrcMgr::C_System_ModuleMap : SrcMgr::C_User_ModuleMap;
   FileID OriginalModuleMapFID = CI.getSourceManager().getOrCreateFileID(
   *OriginalModuleMap, FileCharacter);
-  CI.getPreprocessor().getHeaderSearchInfo().getModuleMap()
-.setInferredModuleAllowedBy(M, OriginalModuleMapFID);
+  CI.getPreprocessor()
+  .getHeaderSearchInfo()
+  .getModuleMap()
+  .setInferredModuleAllowedBy(M, OriginalModuleMapFID);
 }
   }
 

``




https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)

2024-03-21 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 created 
https://github.com/llvm/llvm-project/pull/86216

The `ASTWriter` algorithm for computing affecting module maps uses 
`SourceManager::translateFile()` to get a `FileID` from a `FileEntry`. This is 
slow (O(n)) since the function performs a linear walk over `SLocEntries` until 
it finds one with a matching `FileEntry`.

This patch removes this use of `SourceManager::translateFile()` by tracking 
`FileID` instead of `FileEntry` in couple of places in `ModuleMap`, giving 
`ASTWriter` the desired `FileID` directly. There are no changes required for 
clients that still want a `FileEntry` from `ModuleMap`: the existing APIs 
internally use `SourceManager` to perform the reverse `FileID` to `FileEntry` 
conversion in O(1).

>From 4dd48b40a034edf0b124ab08055a334ad7abd5ba Mon Sep 17 00:00:00 2001
From: Jan Svoboda 
Date: Thu, 21 Mar 2024 14:40:02 -0700
Subject: [PATCH] [clang][modules] Avoid calling expensive
 `SourceManager::translateFile()`

---
 clang/include/clang/Lex/ModuleMap.h   | 15 +++---
 clang/lib/Frontend/FrontendAction.cpp |  7 ++-
 clang/lib/Lex/ModuleMap.cpp   | 66 +++
 clang/lib/Serialization/ASTWriter.cpp | 23 +-
 4 files changed, 61 insertions(+), 50 deletions(-)

diff --git a/clang/include/clang/Lex/ModuleMap.h 
b/clang/include/clang/Lex/ModuleMap.h
index 867cb6eab42f2d..2e28ff6823cb2a 100644
--- a/clang/include/clang/Lex/ModuleMap.h
+++ b/clang/include/clang/Lex/ModuleMap.h
@@ -263,8 +263,8 @@ class ModuleMap {
 Attributes Attrs;
 
 /// If \c InferModules is non-zero, the module map file that allowed
-/// inferred modules.  Otherwise, nullopt.
-OptionalFileEntryRef ModuleMapFile;
+/// inferred modules.  Otherwise, invalid.
+FileID ModuleMapFID;
 
 /// The names of modules that cannot be inferred within this
 /// directory.
@@ -279,8 +279,7 @@ class ModuleMap {
 
   /// A mapping from an inferred module to the module map that allowed the
   /// inference.
-  // FIXME: Consider making the values non-optional.
-  llvm::DenseMap InferredModuleAllowedBy;
+  llvm::DenseMap InferredModuleAllowedBy;
 
   llvm::DenseMap AdditionalModMaps;
 
@@ -618,8 +617,9 @@ class ModuleMap {
   ///
   /// \param Module The module whose module map file will be returned, if 
known.
   ///
-  /// \returns The file entry for the module map file containing the given
-  /// module, or nullptr if the module definition was inferred.
+  /// \returns The FileID for the module map file containing the given module,
+  /// invalid if the module definition was inferred.
+  FileID getContainingModuleMapFileID(const Module *Module) const;
   OptionalFileEntryRef getContainingModuleMapFile(const Module *Module) const;
 
   /// Get the module map file that (along with the module name) uniquely
@@ -631,9 +631,10 @@ class ModuleMap {
   /// of inferred modules, returns the module map that allowed the inference
   /// (e.g. contained 'module *'). Otherwise, returns
   /// getContainingModuleMapFile().
+  FileID getModuleMapFileIDForUniquing(const Module *M) const;
   OptionalFileEntryRef getModuleMapFileForUniquing(const Module *M) const;
 
-  void setInferredModuleAllowedBy(Module *M, OptionalFileEntryRef ModMap);
+  void setInferredModuleAllowedBy(Module *M, FileID ModMapFID);
 
   /// Canonicalize \p Path in a manner suitable for a module map file. In
   /// particular, this canonicalizes the parent directory separately from the
diff --git a/clang/lib/Frontend/FrontendAction.cpp 
b/clang/lib/Frontend/FrontendAction.cpp
index b9fd9b8897b7e7..bc4c901325c7e1 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -535,8 +535,13 @@ static Module *prepareToBuildModule(CompilerInstance &CI,
 if (*OriginalModuleMap != CI.getSourceManager().getFileEntryRefForID(
   CI.getSourceManager().getMainFileID())) {
   M->IsInferred = true;
+  bool IsSystem = false; // TODO: Propagate the real thing here.
+  auto FileCharacter =
+  IsSystem ? SrcMgr::C_System_ModuleMap : SrcMgr::C_User_ModuleMap;
+  FileID OriginalModuleMapFID = CI.getSourceManager().getOrCreateFileID(
+  *OriginalModuleMap, FileCharacter);
   CI.getPreprocessor().getHeaderSearchInfo().getModuleMap()
-.setInferredModuleAllowedBy(M, *OriginalModuleMap);
+.setInferredModuleAllowedBy(M, OriginalModuleMapFID);
 }
   }
 
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 10c475f617d485..eed7eca2e73562 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -648,8 +648,7 @@ 
ModuleMap::findOrCreateModuleForHeaderInUmbrellaDir(FileEntryRef File) {
   UmbrellaModule = UmbrellaModule->Parent;
 
 if (UmbrellaModule->InferSubmodules) {
-  OptionalFileEntryRef UmbrellaModuleMap =
-  getModuleMapFileForUniquing(UmbrellaModule);
+  FileID UmbrellaModuleMap = getModuleMapFileIDForUniquing