[PATCH] D23125: Modules: add command line option to support loading prebuilt modules on demand, without parsing any module map

[Manman Ren via cfe-commits]

manmanren added a comment.

In https://reviews.llvm.org/D23125#510632, @rsmith wrote:

> This doesn't seem like quite the right user interface for this feature. The module cache is volatile, and it's not really reasonable for people to assume they know what is and isn't within it. Instead, it seems like what we want to provide is zero or more paths to directories containing prebuilt .pcm files whose file names are the same as their top-level module names, completely separate from the module cache.
>
> Specifically, rather than spelling this as `-fmodules-use-prebuilt-modules -fmodules-cache-path=X`, I'd suggest spelling it as `-fprebuilt-module-path=X` or similar (which can be repeated to provide multiple search paths). That would then combine orthogonally with `-fno-implicit-modules` and `-fno-implicit-module-maps`, where implicit modules would still use the module cache. Does that make sense for your use case?


What we want is to achieve performance by prebuilding the modules and using @import to load the corresponding pcm without  parsing the module maps. In ASTReader, we want to treat these modules in the same way as the explicit modules.

-fprebuilt-module-path sounds reasonable. Let me see how the implementation goes ...

> You're also missing updates to the modules documentation.


Yes, I will update the document.

Cheers,
Manman


================
Comment at: lib/Frontend/CompilerInstance.cpp:1502-1510
@@ -1477,1 +1501,11 @@
     case ASTReader::Missing: {
+      if (HSOpts.ModulesUsePrebuiltModules) {
+        // We can't rebuild the module without a module map. Throw diagnostics
+        // here if using prebuilt modules.
+        getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
+        << ModuleName
+        << SourceRange(ImportLoc, ModuleNameLoc);
+        ModuleBuildFailed = true;
+        return ModuleLoadResult();
+      }
+
----------------
rsmith wrote:
> You requested that `ReadAST` produces diagnostics for the `OutOfDate` and `Missing` cases when using prebuilt modules, so this diagnostic is redundant.
Yes, I can remove this diagnostic.

================
Comment at: lib/Serialization/ASTReader.cpp:495-498
@@ -494,1 +494,6 @@
 
+static bool isPrebuiltModule(Preprocessor &PP, ModuleKind MK) {
+  return (MK == MK_ExplicitModule || MK == MK_ImplicitModule) &&
+      PP.getHeaderSearchInfo().getHeaderSearchOpts().ModulesUsePrebuiltModules;
+}
+
----------------
rsmith wrote:
> This seems unnecessarily complicated. Perhaps you could make `CompilerInstance::loadModule` treat all prebuilt modules as `MK_ExplicitModule`, or add a third `ModuleKind` for them?
I tried adding a ModuleKind at some point, but somehow it didn't work out. I will take another look at that.


https://reviews.llvm.org/D23125