[PATCH] D112915: WIP: [clang][modules] Granular tracking of includes

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D112915#3104873 <https://reviews.llvm.org/D112915#3104873>, @vsapsai wrote:

> There can be other reasons to keep `IncludeMap` out of `SubmoduleState` but I'm not sure the local submodule visibility is the right reason. I might be reading the code incorrectly but looks like `CurSubmoduleState` is used when local submodule visibility is disabled. The difference is it's tracking the aggregate state instead of per-submodule state. Need to experiment more but so far tracking includes in `SubmoduleState` follows the spirit of local submodule visibility. Though it's not guaranteed it'll work perfectly from the technical perspective.

Yes, `CurSubmoduleState` is being used unconditionally. However, without local submodule visibility enabled, it always points to `NullSubmoduleState`. Only with the feature enabled does it point to the current submodule state (stored in `Submodules`). The change happens in `Preprocessor::{Enter,Leave}Submodule`.

> Also I think we'll need to increase granularity to track other HeaderFileInfo attributes, not just NumIncludes. Don't have a test case to illustrate that right now and no need to change that now but something to keep in mind.

That's interesting. I think `HeaderFileInfo::isImport` should definitely be tracked in the preprocessor, not in `HeaderFileInfo`. The fact that the header was `#import`ed is not an intrinsic property of the file itself, but rather a preprocessor state. Can you think of other fields that don't really belong to `HeaderFileInfo`?

Based on your feedback, I simplified the patch quite a bit. We're no longer copying the include state between submodules. In its current form, this patch essentially moves `HeaderFileInfo::NumIncludes` into `Preprocessor::NumIncludes` and still uses it as the source of truth.
However, we're also tracking `NumIncludes` separately in each submodule and serializing this into the PCM. Instead of merging `NumIncludes` of the whole module when loading it (which we did before), we can merge `NumIncludes` only of the modules we actually import.



================
Comment at: clang/lib/Lex/Preprocessor.cpp:1323
+        auto &ModNumIncludes = SubmoduleIncludeStates[M].NumIncludes;
+        for (unsigned UID = 0; UID <= ModNumIncludes.maxUID(); ++UID) {
+          CurSubmoduleIncludeState->NumIncludes[UID] += ModNumIncludes[UID];
----------------
Iterating over all FileEntries is probably not very efficient, as Volodymyr mentioned. Thinking about how to make this more efficient...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112915