[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Jan Svoboda via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6924a49690ee: [clang][modules] Account for non-affecting 
inputs in `ASTWriter` (authored by jansvoboda11).

Changed prior to commit:
  https://reviews.llvm.org/D136624?vs=472466&id=472488#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

Files:
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/add-remove-irrelevant-module-map.m

Index: clang/test/Modules/add-remove-irrelevant-module-map.m
===
--- clang/test/Modules/add-remove-irrelevant-module-map.m
+++ clang/test/Modules/add-remove-irrelevant-module-map.m
@@ -1,58 +1,32 @@
 // RUN: rm -rf %t && mkdir %t
 // RUN: split-file %s %t
 
-//--- a.modulemap
+//--- a/module.modulemap
 module a {}
 
-//--- b.modulemap
+//--- b/module.modulemap
 module b {}
 
-//--- test-simple.m
-// expected-no-diagnostics
-@import a;
-
-// Build without b.modulemap:
-//
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -fdisable-module-hash \
-// RUN:   -fmodule-map-file=%t/a.modulemap %t/test-simple.m -verify
-// RUN: mv %t/cache %t/cache-without-b
-
-// Build with b.modulemap:
-//
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -fdisable-module-hash \
-// RUN:   -fmodule-map-file=%t/a.modulemap -fmodule-map-file=%t/b.modulemap %t/test-simple.m -verify
-// RUN: mv %t/cache %t/cache-with-b
-
-// Neither PCM file considers 'b.modulemap' an input:
-//
-// RUN: %clang_cc1 -module-file-info %t/cache-without-b/a.pcm | FileCheck %s --check-prefix=CHECK-B
-// RUN: %clang_cc1 -module-file-info %t/cache-with-b/a.pcm | FileCheck %s --check-prefix=CHECK-B
-// CHECK-B-NOT: Input file: {{.*}}b.modulemap
-
-//--- c.modulemap
-module c [no_undeclared_includes] { header "c.h" }
+//--- c/module.modulemap
+module c {}
 
-//--- c.h
-#if __has_include("d.h") // This should use 'd.modulemap' in order to determine that 'd.h'
- // doesn't exist for 'c' because of its '[no_undeclared_includes]'.
-#endif
-
-//--- d.modulemap
-module d { header "d.h" }
-
-//--- d.h
-// empty
+//--- module.modulemap
+module m { header "m.h" }
+//--- m.h
+@import c;
 
-//--- test-no-undeclared-includes.m
+//--- test-simple.m
 // expected-no-diagnostics
-@import c;
+@import m;
+
+// Build modules with the non-affecting "a/module.modulemap".
+// RUN: %clang_cc1 -I %t/a -I %t/b -I %t/c -I %t -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -fdisable-module-hash %t/test-simple.m -verify
+// RUN: mv %t/cache %t/cache-with
 
-// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fdisable-module-hash \
-// RUN:   -fmodule-map-file=%t/c.modulemap -fmodule-map-file=%t/d.modulemap \
-// RUN:   %t/test-no-undeclared-includes.m -verify
+// Build modules without the non-affecting "a/module.modulemap".
+// RUN: rm -rf %t/a/module.modulemap
+// RUN: %clang_cc1 -I %t/a -I %t/b -I %t/c -I %t -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -fdisable-module-hash %t/test-simple.m -verify
+// RUN: mv %t/cache %t/cache-without
 
-// The PCM file considers 'd.modulemap' an input because it affects the compilation,
-// although it doesn't describe the built module or its imports.
-//
-// RUN: %clang_cc1 -module-file-info %t/cache/c.pcm | FileCheck %s --check-prefix=CHECK-D
-// CHECK-D: Input file: {{.*}}d.modulemap
+// Check that the PCM files are bit-for-bit identical.
+// RUN: diff %t/cache-with/m.pcm %t/cache-without/m.pcm
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -161,8 +161,8 @@
 
 namespace {
 
-std::set GetAllModuleMaps(const HeaderSearch &HS,
- Module *RootModule) {
+std::set GetAffectingModuleMaps(const HeaderSearch &HS,
+   Module *RootModule) {
   std::set ModuleMaps{};
   std::set ProcessedModules;
   SmallVector ModulesToProcess{RootModule};
@@ -1477,15 +1477,8 @@
   AddFileID(SM.getMainFileID(), Record);
   Stream.EmitRecord(ORIGINAL_FILE_ID, Record);
 
-  std::set AffectingClangModuleMaps;
-  if (WritingModule) {
-AffectingClangModuleMaps =
-GetAllModuleMaps(PP.getHeaderSearchInfo(), WritingModule);
-  }
-
   WriteInputFiles(Context.SourceMgr,
-  PP.getHeaderSearchInfo().getHeaderSearchOpts(),
-  AffectingClangModuleMaps);
+  PP.getHeaderSearchInfo().getHeaderSearchOpts());
   Stream.ExitBlock();
 }
 
@@ -1503,9 +1496,8 @@
 
 } // namespace
 
-void ASTWriter::WriteInputFiles(
-SourceManager &SourceMgr, HeaderSearchOptions &HSOpts,
-std:

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM, with one suggestion for the test inline.




Comment at: clang/test/Modules/add-remove-irrelevant-module-map.m:22
+// Build modules with the non-affecting "a/module.modulemap".
+// RUN: %clang_cc1 -I %t/a -I %t/b -I %t/c -I %t -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t/cache -fdisable-module-hash 
%t/test-simple.m
+// RUN: mv %t/cache %t/cache-with

Maybe the CC1s should add `-verify` and `test-simple.m` should have `// 
expected-no-diagnostics` to help protect against bitrot?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 472466.
jansvoboda11 marked an inline comment as done.
jansvoboda11 added a comment.

Rebase, decrease nesting, test using `diff`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

Files:
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/add-remove-irrelevant-module-map.m

Index: clang/test/Modules/add-remove-irrelevant-module-map.m
===
--- clang/test/Modules/add-remove-irrelevant-module-map.m
+++ clang/test/Modules/add-remove-irrelevant-module-map.m
@@ -1,58 +1,31 @@
 // RUN: rm -rf %t && mkdir %t
 // RUN: split-file %s %t
 
-//--- a.modulemap
+//--- a/module.modulemap
 module a {}
 
-//--- b.modulemap
+//--- b/module.modulemap
 module b {}
 
-//--- test-simple.m
-// expected-no-diagnostics
-@import a;
-
-// Build without b.modulemap:
-//
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -fdisable-module-hash \
-// RUN:   -fmodule-map-file=%t/a.modulemap %t/test-simple.m -verify
-// RUN: mv %t/cache %t/cache-without-b
-
-// Build with b.modulemap:
-//
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -fdisable-module-hash \
-// RUN:   -fmodule-map-file=%t/a.modulemap -fmodule-map-file=%t/b.modulemap %t/test-simple.m -verify
-// RUN: mv %t/cache %t/cache-with-b
-
-// Neither PCM file considers 'b.modulemap' an input:
-//
-// RUN: %clang_cc1 -module-file-info %t/cache-without-b/a.pcm | FileCheck %s --check-prefix=CHECK-B
-// RUN: %clang_cc1 -module-file-info %t/cache-with-b/a.pcm | FileCheck %s --check-prefix=CHECK-B
-// CHECK-B-NOT: Input file: {{.*}}b.modulemap
-
-//--- c.modulemap
-module c [no_undeclared_includes] { header "c.h" }
-
-//--- c.h
-#if __has_include("d.h") // This should use 'd.modulemap' in order to determine that 'd.h'
- // doesn't exist for 'c' because of its '[no_undeclared_includes]'.
-#endif
-
-//--- d.modulemap
-module d { header "d.h" }
-
-//--- d.h
-// empty
-
-//--- test-no-undeclared-includes.m
-// expected-no-diagnostics
+//--- c/module.modulemap
+module c {}
+
+//--- module.modulemap
+module m { header "m.h" }
+//--- m.h
 @import c;
 
-// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fdisable-module-hash \
-// RUN:   -fmodule-map-file=%t/c.modulemap -fmodule-map-file=%t/d.modulemap \
-// RUN:   %t/test-no-undeclared-includes.m -verify
+//--- test-simple.m
+@import m;
+
+// Build modules with the non-affecting "a/module.modulemap".
+// RUN: %clang_cc1 -I %t/a -I %t/b -I %t/c -I %t -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -fdisable-module-hash %t/test-simple.m
+// RUN: mv %t/cache %t/cache-with
+
+// Build modules without the non-affecting "a/module.modulemap".
+// RUN: rm -rf %t/a/module.modulemap
+// RUN: %clang_cc1 -I %t/a -I %t/b -I %t/c -I %t -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -fdisable-module-hash %t/test-simple.m
+// RUN: mv %t/cache %t/cache-without
 
-// The PCM file considers 'd.modulemap' an input because it affects the compilation,
-// although it doesn't describe the built module or its imports.
-//
-// RUN: %clang_cc1 -module-file-info %t/cache/c.pcm | FileCheck %s --check-prefix=CHECK-D
-// CHECK-D: Input file: {{.*}}d.modulemap
+// Check that the PCM files are bit-for-bit identical.
+// RUN: diff %t/cache-with/m.pcm %t/cache-without/m.pcm
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -161,8 +161,8 @@
 
 namespace {
 
-std::set GetAllModuleMaps(const HeaderSearch &HS,
- Module *RootModule) {
+std::set GetAffectingModuleMaps(const HeaderSearch &HS,
+   Module *RootModule) {
   std::set ModuleMaps{};
   std::set ProcessedModules;
   SmallVector ModulesToProcess{RootModule};
@@ -1477,15 +1477,8 @@
   AddFileID(SM.getMainFileID(), Record);
   Stream.EmitRecord(ORIGINAL_FILE_ID, Record);
 
-  std::set AffectingClangModuleMaps;
-  if (WritingModule) {
-AffectingClangModuleMaps =
-GetAllModuleMaps(PP.getHeaderSearchInfo(), WritingModule);
-  }
-
   WriteInputFiles(Context.SourceMgr,
-  PP.getHeaderSearchInfo().getHeaderSearchOpts(),
-  AffectingClangModuleMaps);
+  PP.getHeaderSearchInfo().getHeaderSearchOpts());
   Stream.ExitBlock();
 }
 
@@ -1503,9 +1496,8 @@
 
 } // namespace
 
-void ASTWriter::WriteInputFiles(
-SourceManager &SourceMgr, HeaderSearchOptions &HSOpts,
-std::set &AffectingClangModuleMaps) {
+void ASTWriter::WriteInputFiles(SourceManager &SourceMgr,
+HeaderSearchOptions &HSOpts) {
   using namespace llvm;
 
   Strea

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D136624#3900593 , @jansvoboda11 
wrote:

> Ah, I forgot to mention this. Building the modules is now only 0.2% slower 
> and importing them 1.2% faster (compared to PCMs with all input files 
> serialized).

Awesome. All upside then :).

I added a few nitpick-y comments inline. I can take another look once you've 
made the PCMs bit-for-bit identical and updated the test (is that happening 
here, or in a separate review)?




Comment at: clang/include/clang/Basic/SourceManager.h:1831-1833
+  bool isLoadedOffset(SourceLocation::UIntTy SLocOffset) const {
+return SLocOffset >= CurrentLoadedOffset;
+  }

jansvoboda11 wrote:
> dexonsmith wrote:
> > The logic for `isLoadedOffset()` suggests that it could maybe be subsumed 
> > with "location past the end"?
> I don't think so - we don't want to adjust loaded offsets. Their invariant is 
> that they grow from 2^31 downwards.
> 
> We do want to adjust local offsets past the last non-affecting file though.
Oops, right!



Comment at: clang/lib/Serialization/ASTWriter.cpp:4538
+  for (unsigned I = 1; I != N; ++I) {
+// Get this source location entry.
+const SrcMgr::SLocEntry *SLoc = &SrcMgr.getLocalSLocEntry(I);

Not sure this comment adds much on top of the code on the next line. A sentence 
before the `for` loop describing the overall approach might be useful though.



Comment at: clang/lib/Serialization/ASTWriter.cpp:4550-4554
+if (isModuleMap(File.getFileCharacteristic()) &&
+!isSystem(File.getFileCharacteristic()) &&
+!AffectingModuleMaps.empty() &&
+AffectingModuleMaps.find(Cache->OrigEntry) ==
+AffectingModuleMaps.end()) {

You can reduce nesting by inverting this condition and `continue`.



Comment at: clang/lib/Serialization/ASTWriter.cpp:4562-4563
+  NonAffectingFileIDs.back().ID == FID.ID - 1) {
+// If the previous file was non-affecting as well, just extend its 
entry
+// with our information.
+NonAffectingFileIDs.back() = FID;

I'd slightly prefer the comment *before* the `if`, due to how folding tends to 
work in editors (see the comment even when the code is folded). Probably relies 
on dropping the `else` (see below).



Comment at: clang/lib/Serialization/ASTWriter.cpp:4568
+NonAffectingFileIDsAdjustments.back() = Count;
+  } else {
+NonAffectingFileIDs.push_back(FID);

I suggest `continue` before the `else` to avoid adding nesting for insertion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 marked 4 inline comments as done.
jansvoboda11 added a comment.

In D136624#3900183 , @dexonsmith 
wrote:

> Partly, trying to dig into why read speeds got slower. But maybe that was 
> noise that went away though when you switched to cycles/instructions?
> Great; looking forward to seeing new numbers.

Ah, I forgot to mention this. Building the modules is now only 0.2% slower and 
importing them 1.2% faster (compared to PCMs with all input files serialized).




Comment at: clang/include/clang/Basic/SourceManager.h:1831-1833
+  bool isLoadedOffset(SourceLocation::UIntTy SLocOffset) const {
+return SLocOffset >= CurrentLoadedOffset;
+  }

dexonsmith wrote:
> The logic for `isLoadedOffset()` suggests that it could maybe be subsumed 
> with "location past the end"?
I don't think so - we don't want to adjust loaded offsets. Their invariant is 
that they grow from 2^31 downwards.

We do want to adjust local offsets past the last non-affecting file though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D136624#3900051 , @jansvoboda11 
wrote:

>> - Is there a change in cycles/instructions when the module cache is hot? 
>> (presumably the common case)
>
> I didn't notice this (but didn't look for it specifically). How could that 
> affect performance for PCM writes?

It wouldn't check write perf, but it's part of the overall build perf impact. 
This being neutral (no regression) could help to justify landing the change 
even when there's a penalty for a cold modules cache. My interest in 
(implicitly-discovered) explicitly-built modules was similar (if that's 
neutral, then a regression here is less critical).

Partly, trying to dig into why read speeds got slower. But maybe that was noise 
that went away though when you switched to cycles/instructions?

> Loaded locations make up 68% of all calls to `getAdjustment()`, so it's good 
> it's the first thing we check for. Around 4% of all calls are for locations 
> before the first non-affecting module. That's the second special case. Around 
> 28% are pointing after the last non-affecting module, and the current 
> revision has a special case for that as well. I think it would make sense to 
> prioritize this check. Only the remaining 0.3% of calls do the actual binary 
> search.

Great; looking forward to seeing new numbers.

(BTW, if you check before/after last non-affecting module, does one of those 
subsume the is-loaded check entirely? Looking at `isLoadedOffset()` makes me 
think it might. If so, maybe you can replace that check with an assertion.)

In D136624#3900051 , @jansvoboda11 
wrote:

>> - Are the PCMs bit-for-bit identical now when a non-affecting module is 
>> added to the input? (If not, why not?)
>
> They are not in the current revision, but I'll create a patch that makes them 
> so (we also need to adjust the `NumCreatedFileIDs`).

Great. Seems like another motivation to land this change (despite a 
regression), as it can impact meta-build perf if/when artifacts are being 
archived/shared in a larger context. Specifically, if the PCM artifacts are 
more stable, then:

- They take up less aggregate space in CAS storage.
- Later build steps get more cache hits.

Overall this patch seems like a good step to me; I don't think 1-2% for a 
single compilation on a cold cache is that bad, assuming a hot cache doesn't 
regress:

- Across a single build, the cache gets hot pretty quickly as most compilations 
reuse the same modules.
- Across incremental builds, most of the cache (at least system modules) will 
(usually) be hot.
- For explicit builds, it sounds like there's no regression anyway.




Comment at: clang/include/clang/Basic/SourceManager.h:1831-1833
+  bool isLoadedOffset(SourceLocation::UIntTy SLocOffset) const {
+return SLocOffset >= CurrentLoadedOffset;
+  }

The logic for `isLoadedOffset()` suggests that it could maybe be subsumed with 
"location past the end"?



Comment at: clang/test/Modules/non-affecting-module-maps-source-locations.m:32
+
+// RUN: %clang_cc1 -I %t/first -I %t/second -I %t/third -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t/cache %t/tu.m -o %t/tu.o

jansvoboda11 wrote:
> dexonsmith wrote:
> > dexonsmith wrote:
> > > This is exercising the code, but it could do one better and check if the 
> > > output PCMs are bit-for-bit identical when we (now) expect them to be.
> > > 
> > > Maybe you could do this by having two run lines: one that includes `-I 
> > > %t/second` and another that doesn't. Then check if the output PCMs are 
> > > equal.
> > (Or if the PCM isn't bit-for-bit identical yet, maybe at least the AST 
> > block should be...)
> Yes, I'll probably drop this test entirely and just check the PCM files are 
> bit-for-bit identical when a non-affecting file is not loaded at all.
That sounds great.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 marked 5 inline comments as done.
jansvoboda11 added inline comments.



Comment at: clang/include/clang/Serialization/ASTWriter.h:449-452
+  /// Exclusive prefix sum of the lengths of preceding non-affecting inputs.
+  std::vector NonAffectingInputOffsetAdjustments;
+  /// Exclusive prefix sum of the count of preceding non-affecting inputs.
+  std::vector NonAffectingInputFileIDAdjustments;

dexonsmith wrote:
> Can you collect a histogram for how big these vectors are? Can we avoid 
> pointer chasing in the common case by making them `SmallVector` of some size 
> during lookup?
> 
Usually 4-6 elements. Making them a `SmallVector` didn't affect 
performance, though.



Comment at: clang/lib/Serialization/ASTWriter.cpp:5282-5283
+
+SourceLocation::UIntTy
+ASTWriter::getAdjustment(SourceLocation::UIntTy Offset) const {
+  if (PP->getSourceManager().isLoadedOffset(Offset) ||

dexonsmith wrote:
> How often does `getAdjustment()` return the same answer in consecutive calls? 
> If at all common, this would likely benefit from a peephole:
> ```
> lang=c++
> Optional ASTWriter::CachedAdjustmentRange;
> Optional ASTWriter::CachedAdjustment;
> 
> SourceLocation::UIntTy
> ASTWriter::getAdjustment(SourceLocation::UIntTy Offset) const {
>   // Check for 0.
>   //
>   // How fast is "isLoadedOffset()"? Can/should we add a peephole, or is it 
> just bit
>   // manipulation? (I seem to remember it checking the high bit or something, 
> but if
>   // it's doing some sort of look up, maybe it should be in the slow path so 
> it can
>   // get cached by LastAdjustment.)
>   if (PP->getSourceManager().isLoadedOffset(Offset) ||
>   NonAffectingInputs.empty())
> return 0;
> 
>   // Check CachedAdjustment.
>   if (CachedAdjustment && CachedAdjustmentRange->includes(Offset))
> return *CachedAdjustment;
> 
>   // Call getAdjustmentSlow, which updates CachedAdjustment and 
> CachedAdjustmentRange.
>   // It's out-of-line so that `getAdjustment` can easily be inlined without 
> inlining
>   // the slow path.
>   //
>   // LastAdjustmentRange would be the size of the "gap" between this 
> adjustment
>   // level and the next one (end would be UINTMAX if it's after the last
>   // non-affecting range).
>   return getAdjustmentSlow(Offset);
> }
> ```
> 
Not that often, see my top-level comment.



Comment at: clang/lib/Serialization/ASTWriter.cpp:5289-5290
+? NonAffectingInputs.end()
+: llvm::lower_bound(NonAffectingInputs,
+PP->getSourceManager().getFileID(Offset));
+  unsigned Idx = std::distance(NonAffectingInputs.begin(), It);

jansvoboda11 wrote:
> dexonsmith wrote:
> > Why do you need to call `getFileID()` here?
> > 
> > Instead, I would expect this to be a search through a range of offsets 
> > (e.g., see my suggestion at https://reviews.llvm.org/D106876#3869247 -- 
> > `DroppedMMs` contains SourceLocations, not FileIDs).
> > 
> > Two benefits:
> > 1. You don't need to call `getFileID()` to look up an offset.
> > 2. You can merge adjacent non-affecting files (shrinking the search/storage 
> > significantly).
> > 
> > 
> My reasoning was that if we search through a range of offsets, we're doing 
> conceptually the same thing as `getFileID()` (which already has some 
> optimizations baked in). Maybe the non-affecting files are indeed adjacent 
> and we'll be able to merge most of them. I'll give it a shot and report back.
This ended up being faster due to merging of non-affecting files. Thanks for 
the suggestion!



Comment at: clang/test/Modules/non-affecting-module-maps-source-locations.m:32
+
+// RUN: %clang_cc1 -I %t/first -I %t/second -I %t/third -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t/cache %t/tu.m -o %t/tu.o

dexonsmith wrote:
> dexonsmith wrote:
> > This is exercising the code, but it could do one better and check if the 
> > output PCMs are bit-for-bit identical when we (now) expect them to be.
> > 
> > Maybe you could do this by having two run lines: one that includes `-I 
> > %t/second` and another that doesn't. Then check if the output PCMs are 
> > equal.
> (Or if the PCM isn't bit-for-bit identical yet, maybe at least the AST block 
> should be...)
Yes, I'll probably drop this test entirely and just check the PCM files are 
bit-for-bit identical when a non-affecting file is not loaded at all.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D136624#3893607 , @dexonsmith 
wrote:

> I'm curious; are system modules allowed to be non-affecting yet, or are they 
> still assumed to be affecting? (It's the system modules that I think are most 
> likely to be adjacent.)

Yes, all the measurements are done with system modules allowed to be 
non-affecting.

> My intuition is that there is likely some peephole that would be quite 
> effective, that might not be useful for general `getFileID()` lookups.
>
> - I already suggested "same as last lookup?"... I'm curious if that'll help. 
> Maybe that's already in `getFileID()`, but now that you've factored out that 
> call, it could be useful to replicate.

I tried that and we seemingly never hit that case. I think that's because these 
two optimizations take precedence:

> - You could also try: "past the the last non-affecting module?"
> - You could also try: "before the first non-affecting module?"

These avoid the binary search for the vast majority of calls to 
`getAdjustment()` with local offsets.

> I suspect you could collect some data to guide this, such as, for loaded 
> locations (you could ignore "local" locations since they already have a 
> peephole):
>
> - Histogram of "loaded" vs. "between" vs. "after" non-affecting modules.
> - Histogram of "same as last" vs. "same as last-1" vs. "different from last 
> 2".
> - [...]

Loaded locations make up 68% of all calls to `getAdjustment()`, so it's good 
it's the first thing we check for. Around 4% of all calls are for locations 
before the first non-affecting module. That's the second special case. Around 
28% are pointing after the last non-affecting module, and the current revision 
has a special case for that as well. I think it would make sense to prioritize 
this check. Only the remaining 0.3% of calls do the actual binary search.

> Other things that might be useful to know:
>
> - What effect is the merging having (or would it have)? (i.e., what's the 
> histogram of "adjacent" non-affecting files? (e.g.: 9 ranges of non-affecting 
> files, with two blocks of 5 files and seven blocks of 1 (which aren't 
> adjacent to any others)))

Big one. We usually get between 70-100 non-affecting module maps, but they 
merge into 4-6 consecutive regions.

> - Is there a change in cycles/instructions when the module cache is hot? 
> (presumably the common case)

I didn't notice this (but didn't look for it specifically). How could that 
affect performance for PCM writes?

> - Are the PCM artifacts smaller?

Yes, we leave out the 70-100 SLocEntries. Nothing much changes otherwise.

> - Are the PCMs bit-for-bit identical now when a non-affecting module is added 
> to the input? (If not, why not?)

They are not in the current revision, but I'll create a patch that makes them 
so (we also need to adjust the `NumCreatedFileIDs`).

> - What's the data for implicitly-discovered, explicitly-built modules?

I didn't measure that. I don't expect much of a difference, though. The scanner 
has an empty AST, so the number of `SourceLocation`s will be pretty low. (Other 
parts of the PCM file, like the metadata, don't write much `SourceManager` 
offsets.) Even if there was a small regression, I'd be fine with it, since 
we'll be moving off of implicit build in the scanner. For the explicit builds 
themselves, this shouldn't affect performance at all. Implicit module map 
search is disabled, all necessary/affecting module maps are deserialized from 
the PCM files or provided on the command-line.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D136624#3893001 , @jansvoboda11 
wrote:

> I tried implementing your suggestion (merging ranges of adjacent 
> non-affecting files and avoiding `FileID` lookups), but the numbers from 
> `-ftime-trace` are very noisy. I got more stable data by measuring clock 
> cycles and instruction counts, but nothing conclusive yet.
>
> Compilation of `CompilerInvocation.cpp` with implicit modules.
>
> - previous approach with vector + `FileID` lookup: +0.64% cycles and +1.68% 
> instructions,
> - current approach with merged `SourceRange`s: +0.38% cycles and +1.11% 
> instructions.
>
> I'll post here as I experiment more and get more data.

Nice; that seems like a bit of an improvement.

I'm curious; are system modules allowed to be non-affecting yet, or are they 
still assumed to be affecting? (It's the system modules that I think are most 
likely to be adjacent.)

My intuition is that there is likely some peephole that would be quite 
effective, that might not be useful for general `getFileID()` lookups.

- I already suggested "same as last lookup?"... I'm curious if that'll help. 
Maybe that's already in `getFileID()`, but now that you've factored out that 
call, it could be useful to replicate.
- You could also try: "past the the last non-affecting module?"
- You could also try: "before the first non-affecting module?"

I suspect you could collect some data to guide this, such as, for loaded 
locations (you could ignore "local" locations since they already have a 
peephole):

- Histogram of "loaded" vs. "between" vs. "after" non-affecting modules.
- Histogram of "same as last" vs. "same as last-1" vs. "different from last 2".
- [...]

Other things that might be useful to know:

- What effect is the merging having (or would it have)? (i.e., what's the 
histogram of "adjacent" non-affecting files? (e.g.: 9 ranges of non-affecting 
files, with two blocks of 5 files and seven blocks of 1 (which aren't adjacent 
to any others)))
- Is there a change in cycles/instructions when the module cache is hot? 
(presumably the common case)
- Are the PCM artifacts smaller?
- Are the PCMs bit-for-bit identical now when a non-affecting module is added 
to the input? (If not, why not?)
- What's the data for implicitly-discovered, explicitly-built modules?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

I tried implementing your suggestion (merging ranges of adjacent non-affecting 
files and avoiding `FileID` lookups), but the numbers from `-ftime-trace` are 
very noisy. I got more stable data by measuring clock cycles and instruction 
counts, but nothing conclusive yet.

Compilation of `CompilerInvocation.cpp` with implicit modules.

- previous approach with vector + `FileID` lookup: +0.64% cycles and +1.68% 
instructions,
- current approach with merged `SourceRange`s: +0.38% cycles and +1.11% 
instructions.

I'll post here as I experiment more and get more data.




Comment at: clang/lib/Serialization/ASTWriter.cpp:5289-5290
+? NonAffectingInputs.end()
+: llvm::lower_bound(NonAffectingInputs,
+PP->getSourceManager().getFileID(Offset));
+  unsigned Idx = std::distance(NonAffectingInputs.begin(), It);

dexonsmith wrote:
> Why do you need to call `getFileID()` here?
> 
> Instead, I would expect this to be a search through a range of offsets (e.g., 
> see my suggestion at https://reviews.llvm.org/D106876#3869247 -- `DroppedMMs` 
> contains SourceLocations, not FileIDs).
> 
> Two benefits:
> 1. You don't need to call `getFileID()` to look up an offset.
> 2. You can merge adjacent non-affecting files (shrinking the search/storage 
> significantly).
> 
> 
My reasoning was that if we search through a range of offsets, we're doing 
conceptually the same thing as `getFileID()` (which already has some 
optimizations baked in). Maybe the non-affecting files are indeed adjacent and 
we'll be able to merge most of them. I'll give it a shot and report back.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 471641.
jansvoboda11 added a comment.

Avoid looking up `FileID` for an offset.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/non-affecting-module-maps-source-locations.m

Index: clang/test/Modules/non-affecting-module-maps-source-locations.m
===
--- /dev/null
+++ clang/test/Modules/non-affecting-module-maps-source-locations.m
@@ -0,0 +1,32 @@
+// This patch tests that non-affecting files are serialized in such way that
+// does not break subsequent source locations (e.g. in serialized pragma
+// diagnostic mappings).
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- tu.m
+#include "first.h"
+
+//--- first/module.modulemap
+module first { header "first.h" }
+//--- first/first.h
+@import second;
+#pragma clang diagnostic ignored "-Wunreachable-code"
+
+//--- second/extra/module.modulemap
+module second_extra {}
+//--- second/module.modulemap
+module second { module sub { header "second_sub.h" } }
+//--- second/second_sub.h
+@import third;
+
+//--- third/module.modulemap
+module third { header "third.h" }
+//--- third/third.h
+#if __has_feature(nullability)
+#endif
+#if __has_feature(nullability)
+#endif
+
+// RUN: %clang_cc1 -I %t/first -I %t/second -I %t/third -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache %t/tu.m -o %t/tu.o
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -2457,11 +2457,12 @@
   SourceLocation Loc = D->getLocation();
   unsigned Index = ID - FirstDeclID;
   if (DeclOffsets.size() == Index)
-DeclOffsets.emplace_back(Loc, Offset, DeclTypesBlockStartOffset);
+DeclOffsets.emplace_back(getAdjustedLocation(Loc), Offset,
+ DeclTypesBlockStartOffset);
   else if (DeclOffsets.size() < Index) {
 // FIXME: Can/should this happen?
 DeclOffsets.resize(Index+1);
-DeclOffsets[Index].setLocation(Loc);
+DeclOffsets[Index].setLocation(getAdjustedLocation(Loc));
 DeclOffsets[Index].setBitOffset(Offset, DeclTypesBlockStartOffset);
   } else {
 llvm_unreachable("declarations should be emitted in ID order");
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1468,23 +1468,16 @@
 
 Record.clear();
 Record.push_back(ORIGINAL_FILE);
-Record.push_back(SM.getMainFileID().getOpaqueValue());
+AddFileID(SM.getMainFileID(), Record);
 EmitRecordWithPath(FileAbbrevCode, Record, MainFile->getName());
   }
 
   Record.clear();
-  Record.push_back(SM.getMainFileID().getOpaqueValue());
+  AddFileID(SM.getMainFileID(), Record);
   Stream.EmitRecord(ORIGINAL_FILE_ID, Record);
 
-  std::set AffectingModuleMaps;
-  if (WritingModule) {
-AffectingModuleMaps =
-GetAllModuleMaps(PP.getHeaderSearchInfo(), WritingModule);
-  }
-
   WriteInputFiles(Context.SourceMgr,
-  PP.getHeaderSearchInfo().getHeaderSearchOpts(),
-  AffectingModuleMaps);
+  PP.getHeaderSearchInfo().getHeaderSearchOpts());
   Stream.ExitBlock();
 }
 
@@ -1502,9 +1495,8 @@
 
 } // namespace
 
-void ASTWriter::WriteInputFiles(
-SourceManager &SourceMgr, HeaderSearchOptions &HSOpts,
-std::set &AffectingModuleMaps) {
+void ASTWriter::WriteInputFiles(SourceManager &SourceMgr,
+HeaderSearchOptions &HSOpts) {
   using namespace llvm;
 
   Stream.EnterSubblock(INPUT_FILES_BLOCK_ID, 4);
@@ -1544,15 +1536,9 @@
 if (!Cache->OrigEntry)
   continue;
 
-if (isModuleMap(File.getFileCharacteristic()) &&
-!isSystem(File.getFileCharacteristic()) &&
-!AffectingModuleMaps.empty() &&
-AffectingModuleMaps.find(Cache->OrigEntry) ==
-AffectingModuleMaps.end()) {
-  SkippedModuleMaps.insert(Cache->OrigEntry);
-  // Do not emit modulemaps that do not affect current module.
+// Do not emit inputs that do not affect current module.
+if (!IsSLocAffecting[I])
   continue;
-}
 
 InputFileEntry Entry;
 Entry.File = Cache->OrigEntry;
@@ -2046,7 +2032,6 @@
 // Record the offset of this source-location entry.
 uint64_t Offset = Stream.GetCurrentBitNo() - SLocEntryOffsetsBase;
 assert((Offset >> 32) == 0 && "SLocEntry offset too large");
-SLocEntryOffsets.push_back(Offset);
 
 // Figure out which record code to use.
 unsigned Code;
@@ -2061,17 +2046,15 @@
 Record.clear();
 Record.push_b

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/test/Modules/non-affecting-module-maps-source-locations.m:32
+
+// RUN: %clang_cc1 -I %t/first -I %t/second -I %t/third -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t/cache %t/tu.m -o %t/tu.o

dexonsmith wrote:
> This is exercising the code, but it could do one better and check if the 
> output PCMs are bit-for-bit identical when we (now) expect them to be.
> 
> Maybe you could do this by having two run lines: one that includes `-I 
> %t/second` and another that doesn't. Then check if the output PCMs are equal.
(Or if the PCM isn't bit-for-bit identical yet, maybe at least the AST block 
should be...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D136624#3888387 , @jansvoboda11 
wrote:

> I tried optimizing this patch a bit. Instead of creating compact data 
> structure and using binary search to find the preceding non-affecting file, I 
> now store the adjustment information for each `FileID` in a vector. During 
> deserialization, `FileID` is simply used as an index into `SLocEntryInfos`. 
> That didn't yield any measurable improvement in performance, though. I think 
> the regression must be coming from the `SourceLocation`/`Offset` to `FileID` 
> translation.
>
> I don't see any obvious way to work around that. 
> `SourceManager::getFileIDLocal()` already implements some optimizations that 
> makes accessing nearby offsets fast. A separate `SourceManager` would avoid 
> this bottleneck, but I'm not sure how much work that would entail (seems 
> substantial).
>
> @Bigcheese LMK if you're fine with the performance implications here.

I don't think you need to call `getFileID()` inside `getAdjustment()`. There is 
also an opportunity for a peephole, if `getAdjustment()` remains expensive.

I've left some comments on the previous version of the patch since it's not 
obvious to me how to avoid the `getFileID()` call in the new version.

In D136624#3888097 , @jansvoboda11 
wrote:

> The serialization slowdown I understand, but I expected deserialization to 
> get faster, since we now have less of `SourceManager` to look through.

Seems worth digging into the deserialization regression. Does the PCM actually 
get smaller and the ranges more condensed?

One quick test would be to manufacture a situation where two output PCMs would 
previously have different non-affecting inputs, but now should be bit-for-bit 
identical. Are they, in fact, bit-for-bit identical? If not, maybe there's 
something funny to look into...




Comment at: clang/include/clang/Serialization/ASTWriter.h:449-452
+  /// Exclusive prefix sum of the lengths of preceding non-affecting inputs.
+  std::vector NonAffectingInputOffsetAdjustments;
+  /// Exclusive prefix sum of the count of preceding non-affecting inputs.
+  std::vector NonAffectingInputFileIDAdjustments;

Can you collect a histogram for how big these vectors are? Can we avoid pointer 
chasing in the common case by making them `SmallVector` of some size during 
lookup?




Comment at: clang/lib/Serialization/ASTWriter.cpp:2054
 // Starting offset of this entry within this module, so skip the dummy.
-Record.push_back(SLoc->getOffset() - 2);
+Record.push_back(getAdjustedOffset(SLoc->getOffset()) - 2);
 if (SLoc->isFile()) {

Can we shift this `getAdjustedOffset()` computation to after deciding whether 
to skip the record?



Comment at: clang/lib/Serialization/ASTWriter.cpp:5282-5283
+
+SourceLocation::UIntTy
+ASTWriter::getAdjustment(SourceLocation::UIntTy Offset) const {
+  if (PP->getSourceManager().isLoadedOffset(Offset) ||

How often does `getAdjustment()` return the same answer in consecutive calls? 
If at all common, this would likely benefit from a peephole:
```
lang=c++
Optional ASTWriter::CachedAdjustmentRange;
Optional ASTWriter::CachedAdjustment;

SourceLocation::UIntTy
ASTWriter::getAdjustment(SourceLocation::UIntTy Offset) const {
  // Check for 0.
  //
  // How fast is "isLoadedOffset()"? Can/should we add a peephole, or is it 
just bit
  // manipulation? (I seem to remember it checking the high bit or something, 
but if
  // it's doing some sort of look up, maybe it should be in the slow path so it 
can
  // get cached by LastAdjustment.)
  if (PP->getSourceManager().isLoadedOffset(Offset) ||
  NonAffectingInputs.empty())
return 0;

  // Check CachedAdjustment.
  if (CachedAdjustment && CachedAdjustmentRange->includes(Offset))
return *CachedAdjustment;

  // Call getAdjustmentSlow, which updates CachedAdjustment and 
CachedAdjustmentRange.
  // It's out-of-line so that `getAdjustment` can easily be inlined without 
inlining
  // the slow path.
  //
  // LastAdjustmentRange would be the size of the "gap" between this adjustment
  // level and the next one (end would be UINTMAX if it's after the last
  // non-affecting range).
  return getAdjustmentSlow(Offset);
}
```




Comment at: clang/lib/Serialization/ASTWriter.cpp:5289-5290
+? NonAffectingInputs.end()
+: llvm::lower_bound(NonAffectingInputs,
+PP->getSourceManager().getFileID(Offset));
+  unsigned Idx = std::distance(NonAffectingInputs.begin(), It);

Why do you need to call `getFileID()` here?

Instead, I would expect this to be a search through a range of offsets (e.g., 
see my suggestion at https://reviews.llvm.org/D106876#3869247 -- `DroppedMMs` 
contains SourceLocations, not FileIDs).

Two benefits:
1. You don't need 

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 471152.
jansvoboda11 added a comment.

New version with flat vector + `FileID` indices; replacing the previous compact 
representation & binary search 
approach


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/non-affecting-module-maps-source-locations.m

Index: clang/test/Modules/non-affecting-module-maps-source-locations.m
===
--- /dev/null
+++ clang/test/Modules/non-affecting-module-maps-source-locations.m
@@ -0,0 +1,32 @@
+// This patch tests that non-affecting files are serialized in such way that
+// does not break subsequent source locations (e.g. in serialized pragma
+// diagnostic mappings).
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- tu.m
+#include "first.h"
+
+//--- first/module.modulemap
+module first { header "first.h" }
+//--- first/first.h
+@import second;
+#pragma clang diagnostic ignored "-Wunreachable-code"
+
+//--- second/extra/module.modulemap
+module second_extra {}
+//--- second/module.modulemap
+module second { module sub { header "second_sub.h" } }
+//--- second/second_sub.h
+@import third;
+
+//--- third/module.modulemap
+module third { header "third.h" }
+//--- third/third.h
+#if __has_feature(nullability)
+#endif
+#if __has_feature(nullability)
+#endif
+
+// RUN: %clang_cc1 -I %t/first -I %t/second -I %t/third -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache %t/tu.m -o %t/tu.o
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -2457,11 +2457,12 @@
   SourceLocation Loc = D->getLocation();
   unsigned Index = ID - FirstDeclID;
   if (DeclOffsets.size() == Index)
-DeclOffsets.emplace_back(Loc, Offset, DeclTypesBlockStartOffset);
+DeclOffsets.emplace_back(getAdjustedLocation(Loc), Offset,
+ DeclTypesBlockStartOffset);
   else if (DeclOffsets.size() < Index) {
 // FIXME: Can/should this happen?
 DeclOffsets.resize(Index+1);
-DeclOffsets[Index].setLocation(Loc);
+DeclOffsets[Index].setLocation(getAdjustedLocation(Loc));
 DeclOffsets[Index].setBitOffset(Offset, DeclTypesBlockStartOffset);
   } else {
 llvm_unreachable("declarations should be emitted in ID order");
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1468,12 +1468,12 @@
 
 Record.clear();
 Record.push_back(ORIGINAL_FILE);
-Record.push_back(SM.getMainFileID().getOpaqueValue());
+AddFileID(SM.getMainFileID(), Record);
 EmitRecordWithPath(FileAbbrevCode, Record, MainFile->getName());
   }
 
   Record.clear();
-  Record.push_back(SM.getMainFileID().getOpaqueValue());
+  AddFileID(SM.getMainFileID(), Record);
   Stream.EmitRecord(ORIGINAL_FILE_ID, Record);
 
   std::set AffectingModuleMaps;
@@ -1483,8 +1483,7 @@
   }
 
   WriteInputFiles(Context.SourceMgr,
-  PP.getHeaderSearchInfo().getHeaderSearchOpts(),
-  AffectingModuleMaps);
+  PP.getHeaderSearchInfo().getHeaderSearchOpts());
   Stream.ExitBlock();
 }
 
@@ -1502,9 +1501,8 @@
 
 } // namespace
 
-void ASTWriter::WriteInputFiles(
-SourceManager &SourceMgr, HeaderSearchOptions &HSOpts,
-std::set &AffectingModuleMaps) {
+void ASTWriter::WriteInputFiles(SourceManager &SourceMgr,
+HeaderSearchOptions &HSOpts) {
   using namespace llvm;
 
   Stream.EnterSubblock(INPUT_FILES_BLOCK_ID, 4);
@@ -1536,6 +1534,10 @@
 const SrcMgr::SLocEntry *SLoc = &SourceMgr.getLocalSLocEntry(I);
 assert(&SourceMgr.getSLocEntry(FileID::get(I)) == SLoc);
 
+// Do not emit inputs that do not affect current module.
+if (!SLocEntryInfos[I].Affecting)
+  continue;
+
 // We only care about file entries that were not overridden.
 if (!SLoc->isFile())
   continue;
@@ -1544,16 +1546,6 @@
 if (!Cache->OrigEntry)
   continue;
 
-if (isModuleMap(File.getFileCharacteristic()) &&
-!isSystem(File.getFileCharacteristic()) &&
-!AffectingModuleMaps.empty() &&
-AffectingModuleMaps.find(Cache->OrigEntry) ==
-AffectingModuleMaps.end()) {
-  SkippedModuleMaps.insert(Cache->OrigEntry);
-  // Do not emit modulemaps that do not affect current module.
-  continue;
-}
-
 InputFileEntry Entry;
 Entry.File = Cache->OrigEntry;
 Entry.IsSystemFile = isSystem(File.getFileCharacteristic());
@@ -2043,6 +2035,10 @@
 FileID FID = Fil

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

I tried optimizing this patch a bit. Instead of creating compact data structure 
and using binary search to find the preceding non-affecting file, I now store 
the adjustment information for each `FileID` in a vector. During 
deserialization, `FileID` is simply used as an index into `SLocEntryInfos`. 
That didn't yield any measurable improvement in performance, though. I think 
the regression must be coming from the `SourceLocation`/`Offset` to `FileID` 
translation.

I don't see any obvious way to work around that. 
`SourceManager::getFileIDLocal()` already implements some optimizations that 
makes accessing nearby offsets fast. A separate `SourceManager` would avoid 
this bottleneck, but I'm not sure how much work that would entail (seems 
substantial).

@Bigcheese LMK if you're fine with the performance implications here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D136624#3880526 , @Bigcheese wrote:

> This looks reasonable. Have you measured the performance impact of this 
> change?

I have done comparison between this patch and 
https://github.com/apple/llvm-project/pull/5451 (that, instead of leaving 
non-affecting input files out, serializes one extra bit saying whether they are 
affecting or not). Both versions carried some additional patches that refine 
the computation of non-affecting module maps, that I plan to upstream shortly. 
I used Clang's `-ftime-trace` on a reasonably large project (1680 TUs, 638 
implicitly-built modules). The results say that serialization got 7.09% slower, 
and deserialization 1.01% slower. The serialization slowdown I understand, but 
I expected deserialization to get faster, since we now have less of 
`SourceManager` to look through. Overall, PCM compilation got 4.35% slower.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

This looks reasonable. Have you measured the performance impact of this change?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 created this revision.
jansvoboda11 added reviewers: dexonsmith, Bigcheese, vsapsai, ilyakuteev.
Herald added subscribers: ributzka, mgrang.
Herald added a project: All.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

In D106876 , we stopped serializing module 
map files that didn't affect compilation of the current module.

However, since each `SourceLocation` is simply an offset into `SourceManager`'s 
global buffer of concatenated input files in, these need to be adjusted during 
serialization. Otherwise, they can incorrectly point after the buffer or into 
subsequent input file.

This patch starts adjusting `SourceLocation`s, `FileID`s and other 
`SourceManager` offsets in `ASTWriter`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136624

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/non-affecting-module-maps-source-locations.m

Index: clang/test/Modules/non-affecting-module-maps-source-locations.m
===
--- /dev/null
+++ clang/test/Modules/non-affecting-module-maps-source-locations.m
@@ -0,0 +1,32 @@
+// This patch tests that non-affecting files are serialized in such way that
+// does not break subsequent source locations (e.g. in serialized pragma
+// diagnostic mappings).
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- tu.m
+#include "first.h"
+
+//--- first/module.modulemap
+module first { header "first.h" }
+//--- first/first.h
+@import second;
+#pragma clang diagnostic ignored "-Wunreachable-code"
+
+//--- second/extra/module.modulemap
+module second_extra {}
+//--- second/module.modulemap
+module second { module sub { header "second_sub.h" } }
+//--- second/second_sub.h
+@import third;
+
+//--- third/module.modulemap
+module third { header "third.h" }
+//--- third/third.h
+#if __has_feature(nullability)
+#endif
+#if __has_feature(nullability)
+#endif
+
+// RUN: %clang_cc1 -I %t/first -I %t/second -I %t/third -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache %t/tu.m -o %t/tu.o
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -2457,11 +2457,12 @@
   SourceLocation Loc = D->getLocation();
   unsigned Index = ID - FirstDeclID;
   if (DeclOffsets.size() == Index)
-DeclOffsets.emplace_back(Loc, Offset, DeclTypesBlockStartOffset);
+DeclOffsets.emplace_back(getAdjustedLocation(Loc), Offset,
+ DeclTypesBlockStartOffset);
   else if (DeclOffsets.size() < Index) {
 // FIXME: Can/should this happen?
 DeclOffsets.resize(Index+1);
-DeclOffsets[Index].setLocation(Loc);
+DeclOffsets[Index].setLocation(getAdjustedLocation(Loc));
 DeclOffsets[Index].setBitOffset(Offset, DeclTypesBlockStartOffset);
   } else {
 llvm_unreachable("declarations should be emitted in ID order");
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -161,8 +161,8 @@
 
 namespace {
 
-std::set GetAllModuleMaps(const HeaderSearch &HS,
- Module *RootModule) {
+std::set GetAffectingModuleMaps(const HeaderSearch &HS,
+   Module *RootModule) {
   std::set ModuleMaps{};
   std::set ProcessedModules;
   SmallVector ModulesToProcess{RootModule};
@@ -1468,23 +1468,16 @@
 
 Record.clear();
 Record.push_back(ORIGINAL_FILE);
-Record.push_back(SM.getMainFileID().getOpaqueValue());
+AddFileID(SM.getMainFileID(), Record);
 EmitRecordWithPath(FileAbbrevCode, Record, MainFile->getName());
   }
 
   Record.clear();
-  Record.push_back(SM.getMainFileID().getOpaqueValue());
+  AddFileID(SM.getMainFileID(), Record);
   Stream.EmitRecord(ORIGINAL_FILE_ID, Record);
 
-  std::set AffectingModuleMaps;
-  if (WritingModule) {
-AffectingModuleMaps =
-GetAllModuleMaps(PP.getHeaderSearchInfo(), WritingModule);
-  }
-
   WriteInputFiles(Context.SourceMgr,
-  PP.getHeaderSearchInfo().getHeaderSearchOpts(),
-  AffectingModuleMaps);
+  PP.getHeaderSearchInfo().getHeaderSearchOpts());
   Stream.ExitBlock();
 }
 
@@ -1502,9 +1495,8 @@
 
 } // namespace
 
-void ASTWriter::WriteInputFiles(
-SourceManager &SourceMgr, HeaderSearchOptions &HSOpts,
-std::set &AffectingModuleMaps) {
+void ASTWriter::WriteInputFiles(SourceManager &SourceMgr,
+HeaderSearchOptions &HSOpts) {
   using namespace llvm;
 
   Stream.EnterSubblock(INPUT_FILES_