[PATCH] D111560: [clang][modules] Cache loads of explicit modules imported by PCH

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D111560#3056021 <https://reviews.llvm.org/D111560#3056021>, @dexonsmith wrote:

> In D111560#3055578 <https://reviews.llvm.org/D111560#3055578>, @jansvoboda11 wrote:
>
>> Note: Another approach to fixing this might be to cache the module load results while loading the PCH too.
>
> Can you share why you chose this approach instead, and which do you think makes sense long term?

I assumed there was a reason this is not being done for PCHs that I can't see.

If that idea seems workable to you, I can give it a try and see if it leads to cleaner code. Conceptually, I think it would make sense to treat both PCHs and PCMs the same in this regard.

It would be nice to have @rsmith's opinion, since I think he originally implemented this.



================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:41
   for (const PrebuiltModuleDep &PMD : PrebuiltModuleDeps) {
-    Args.push_back("-fmodule-file=" + PMD.ModuleName + "=" + PMD.PCMFile);
+    Args.push_back("-fmodule-file=" + PMD.PCMFile);
     Args.push_back("-fmodule-map-file=" + PMD.ModuleMapFile);
----------------
dexonsmith wrote:
> Why drop the `<name>=` argument? Isn't it better to keep that information, rather than forcing the reader to crack open the file to know what's inside?
I think it's nice to be consistent. We already pass PCM files in the `-fmodule-file=<path>` form for:
* all dependencies when precompiling a module,
* non-PCH dependencies dependencies when compiling a TU.
The `-fmodule-file=<name>=<path>` format is handled lazily (done via header search), which might be less efficient (see 0f99d6a4413e3da6ec242c0ab715d6699045dea8). We don't need laziness, since we know we'll need to open the file anyways.

I'm open to revising this decision later as part of performance tuning, but for now, I'd like this to be consistent across the board.

Bonus points: it forces me to fix a Clang bug.

WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111560