Llvm modules on demand bmi #71773
Llvm modules on demand bmi #71773
Conversation
We want to be able to decide if a given compilation should emit a BMI if an appropriate module line is found. We must not confuse these cases with an implementation that implicitly pulls in its respective interface. Differential Revision: https://reviews.llvm.org/D118895
This provides a base implementation for the ability to emit binary module interfaces in the same compilation as generating object or other output. This part defers opening the BMI PCM file until it is actually needed. At this point we will have parsed the TU and therefore can determine if this job should emit both the BMI and some other artefact (in many cases an object file). This initial implementation assumes that the output filename for the potential BMI will be known when the job is created. Differential Revision: https://reviews.llvm.org/D118896
This adds an AST consumer to the pipelines for 'regular' compilation jobs which can generate a binary module interface (BMI) when one is required by the source, rather than making this a separate step. We only write output if a module interface is required for the source (e.g. it contains an exported named module). This initial version has a simplistic approach to determining a default path and name for the binary module interface. The patch is preparatory work for other opportunites - for example that with distinct channels for codegen and BMI output, we can now consider actions to prune unnecessary content from the BMIs. Differential Revision: https://reviews.llvm.org/D118898
With the ability to create compiled module interfaces 'on demand' when the relevant keywords are encountered, we need some mechanism to determine the output file name to be used for such modules. In the most simplistic case, we can choose to name the module CMI based on the name of the source file. However, in many cases that is likely to be an over- simplification. The intent is that this on-demand facility would be used with P1184 or some similar scheme - but that can be abstracted behind some query of the the module loader or module mapper. As a proof-of-principle we re (maybe ab-)use the fmodule=name=path command line options to look up the file path given the module name. Differential Revision: https://reviews.llvm.org/D118899
You can test this locally with the following command:git-clang-format --diff 21861991e760e7e845dc1be5b804c950543d699a 6fa533ecaef0f75c87554fb646b0e5dca8ea7255 -- clang/include/clang/CodeGen/ObjectFilePCHContainerOperations.h clang/include/clang/Sema/Sema.h clang/include/clang/Serialization/ASTWriter.h clang/include/clang/Serialization/PCHContainerOperations.h clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp clang/lib/Frontend/FrontendAction.cpp clang/lib/Serialization/GeneratePCH.cpp clang/lib/Serialization/PCHContainerOperations.cppView the diff from clang-format here.diff --git a/clang/lib/Frontend/FrontendAction.cpp b/clang/lib/Frontend/FrontendAction.cpp
index bfe112ff1..f6794a4f4 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -197,15 +197,16 @@ FrontendAction::CreateWrappedASTConsumer(CompilerInstance &CI,
// to consider the case in which a BMI is explicitly the main output of the
// compilation.
InputKind IK = getCurrentFileKind();
- bool EmitBMI = CI.getLangOpts().CPlusPlusModules &&
- IK.getFormat() == InputKind::Format::Source &&
- !this->usesPreprocessorOnly() &&
- (CI.getFrontendOpts().ProgramAction == frontend::EmitObj ||
- CI.getFrontendOpts().ProgramAction == frontend::EmitAssembly ||
- CI.getFrontendOpts().ProgramAction == frontend::EmitBC ||
- CI.getFrontendOpts().ProgramAction == frontend::EmitLLVM ||
- CI.getFrontendOpts().ProgramAction == frontend::EmitLLVMOnly ||
- CI.getFrontendOpts().ProgramAction == frontend::EmitCodeGenOnly);
+ bool EmitBMI =
+ CI.getLangOpts().CPlusPlusModules &&
+ IK.getFormat() == InputKind::Format::Source &&
+ !this->usesPreprocessorOnly() &&
+ (CI.getFrontendOpts().ProgramAction == frontend::EmitObj ||
+ CI.getFrontendOpts().ProgramAction == frontend::EmitAssembly ||
+ CI.getFrontendOpts().ProgramAction == frontend::EmitBC ||
+ CI.getFrontendOpts().ProgramAction == frontend::EmitLLVM ||
+ CI.getFrontendOpts().ProgramAction == frontend::EmitLLVMOnly ||
+ CI.getFrontendOpts().ProgramAction == frontend::EmitCodeGenOnly);
// If there are no registered plugins and we do not need to emit a BMI, we
// do not need to wrap the consumer in a MultiplexConsumer.
@@ -225,8 +226,8 @@ FrontendAction::CreateWrappedASTConsumer(CompilerInstance &CI,
// statement). We do not open this on the output stream, but provide it
// as a fallback.
// The default here is to output the pcm alongside the main output.
- std::string XOut
- = llvm::sys::path::parent_path(CI.getFrontendOpts().OutputFile).str();
+ std::string XOut =
+ llvm::sys::path::parent_path(CI.getFrontendOpts().OutputFile).str();
std::string XIn = llvm::sys::path::filename(InFile).str();
if (!XOut.empty()) {
XOut += llvm::sys::path::get_separator();
@@ -245,19 +246,19 @@ FrontendAction::CreateWrappedASTConsumer(CompilerInstance &CI,
// Add a job to build the CMI from the AST.
// ??? : change the CTOR flags to note that this is a CXX20 module?
Consumers.push_back(std::make_unique<PCHGenerator>(
- CI.getPreprocessor(), CI.getModuleCache(), XOut, Sysroot, Buffer,
- CI.getFrontendOpts().ModuleFileExtensions,
- /*AllowASTWithErrors=*/false,
- /*IncludeTimestamps=*/false,
- /*BuildingImplicitModule=*/false,
- /*ShouldCacheASTInMemory=*/false,
- /*IsForBMI=*/true));
+ CI.getPreprocessor(), CI.getModuleCache(), XOut, Sysroot, Buffer,
+ CI.getFrontendOpts().ModuleFileExtensions,
+ /*AllowASTWithErrors=*/false,
+ /*IncludeTimestamps=*/false,
+ /*BuildingImplicitModule=*/false,
+ /*ShouldCacheASTInMemory=*/false,
+ /*IsForBMI=*/true));
// This writes the CMI (if one is needed), but does not open the output
// file unless/until it is required.
- Consumers.push_back(CI.getPCHContainerWriter()
- .CreatePCHDeferredContainerGenerator(
- CI, std::string(InFile), XOut, std::move(OS), Buffer));
+ Consumers.push_back(
+ CI.getPCHContainerWriter().CreatePCHDeferredContainerGenerator(
+ CI, std::string(InFile), XOut, std::move(OS), Buffer));
}
// Collect the list of plugins that go before the main action (in Consumers)
|
|
This is a little bit far from what I imaged. There are 2 things in the patch. One is to generate the BMI and the object file in one phase (phase here means preprocess, precompile, compile, ...). Another is to allow us to generate BMI from a The first thing is in my TODO too. But my consideration is more about efficiency. Currently, the one phase compilation model (I found the term But after we introduced thin BMI, it looks inefficient to write the AST twice. So it is on my TODO list after we land the thin BMI patch. BTW, I think we should do thin in CodeGen action instead of hacking on WrappedASTConsumer. For the second thing, I am curious that if it is necessary now? Or what will it block? I mean, the build systems or the cmake, require to mark the module unit ahead of time. Then the build systems will pass And to me, the current mechanism for
I believe we should do this in ASTWriters. Also this should be part of thin BMI. |
This is the main point of the patch - to do this efficiently.
Please do not become distracted by this, it is a side-effect of the main purpose, if the community wants to have modular files with some specific suffix, the patch still works the same.
I am curious as to why you think that the multiplex AST consumer is a hack - it seems to be designed exactly for this purpose and existed already (i.e. not part of this patch).
I think you are getting distracted by the suffix; that is only a side-effect of this patch, not its primary purpose.
I have always viewed that as a temporary work-around because we could not generate both artefacts from one compiler invocation.
We do not need two mechanisms, .cppm can take the same path as any other suffix.
I am strongly against doing more semantic work in the AST reader/writer; that is just compounding existing layering violations that are already hurting us.
I am not sure what you mean here - the full AST is required for code-gen - we can only thin AST either on a separate path (as in this patch) or as a separate step. |
Got it. The we can be more focused.
It is not about multiplex AST consumer. It is about WrappedASTConsumer. It is designed for plugins. Also it is a private member function of FrontendAction, the base of frontend actions. I think we should perform new behaviors in sub-actions. It looks not good to perform semantical analysis in FrontendAction... Concretely, I think we need to do this in CodeGenAction.
Then it implies that we need to discard a bunch of existing codes handling
Agreed in the higher level. But that requires us to implement at least new AST writers.
I mean it should be successors of #71622. Concretely, now we reduce the function definition in https://github.com/llvm/llvm-project/pull/71622/files#diff-125f472e690aa3d973bc42aa3c5d580226c5c47661551aca2889f960681aa64dR321. |
Hm, according to https://clang.llvm.org/docs/StandardCPlusPlusModules.html this can already be achieved with the
Again, just want to clarify: as I understand it, this patch solves the scaling issue Ben reported (#60996) but without the thin/fat BMI complications, correct? |
The difference is about the efficiency and the interfaces doesn't change a lot. Previously, in the one phase compilation mode, what clang did actually is: That said we compile |
|
Let us try to determine fundamental constraints. To produce a BMI (David's ImplementationBMI) that can generate the object file, it must contain all the AST. this is equivalent to hypothetical We can now take that pcm and generate both and object and an InterfaceBMI - by applying filters to the content and writing out a new BMI with the reduced content. (hypothetical command lines) From my perspective that seems to be requiring now three process launches to generate two artefacts, plus we have to save the Implementation.pcm and probably distribute it to different build nodes. The proposition of the patches here is (in the most common general case) to remove the step of generating the ImplementationBMI and to pipeline the filtering to the AST consumer that generates the Interface BMI. Whether we re-use wrapper code or make some new code is an implementation detail. It does not actually prevent you from taking the two-phase approach ( currently , We might also have a version where the implementation PCM could be consumed to generate both object and Interface PCM from a single invocation - but that is still two processes + resources and saving the intermediate Impl. PCM I am generally concerned that we seem to be coming up with designs that require more and more process launches and corresponding memory footprint etc. edit: and note that the production of a minimised BMI for the interface is a long-term team objective - it is not a new requirement introduced here. |
That is the intent. |
Agreed. This is what I want too.
It doesn't prevent the two-phase compilation model indeed. But it introduces a new way about how we produce BMIs. The new way skipped the part of jobs we did in drivers for In my mind, the proper solution is to introduce a new frontend action that combines GenerateModuleInterfaceAction and CodeGenAction. Then in the driver part, we generate the new action in the precompile phase for some combination of the input then we can skip the |
It allows us to produce a new kind of BMI - that carries a minimised content, applicable to the interface, otherwise it is no different to the case where two command lines are needed to produce an object and BMI.. We can also produce the old "full implementation content" BMI if there is some reason to do so.
I'm a bit confused by what you are saying here; the driver does not do any of the work, it simply prepares cc1 commands that are then executed by the compiler. In the current case, we prepare two command lines one that builds an Implementation BMI and one that consumes that to produce an object. We can simplify the driver because now it only needs to prepare one command line (even in the case that we decide to emulate the existing scheme by executing a The difficulty that I have pointed out is that if we preserve the existing scheme but want an Interface BMI - we then have to produce a third compile line in the driver that takes the Implementation BMI and produces the Interface BMI from it. We cannot avoid producing the intermediate BMI here because the jobs are created by the driver and executed by the compiler and we need to Implementation BMI to produce the object.
Initially, that seems like a great idea - until you look at how many possible compile jobs can combine with production of a BMI - for example,
We no longer need the driver to try to generate multiple steps, so it does not need to skip anything. |
This is what thin BMI or interface BMI does.
Oh, this may be the root of the divergence here. In my mind, we can make it without producing new compile jobs. I've already looked at the code. We can avoid producing the intermediate BMI by skipping some phases in the drivers. |
Agreed - but the actual process of producing the interface BMI means taking the "full" AST (which is necessary to generate the object) and reducing it by applying suitable filters [like in my diagram]. This is done in an instance of the front end - it has either to be provided with the full AST or have a multiplexer that puts the full AST to code-gen and then applies filtering to the other path.
I still do not understand the point here: the driver is not doing the work; the driver is preparing cc1 command lines - skipping phases does not achieve anything unless the front end is capable of producing both the BMI and object at the same time (any other solution means materialising the full BMI as an entity). |
Yeah, the full AST is necessary to produce the object file. And the ASTWriters can choose to not write parts of the AST to disk.
My point is that we need to introduce a new frontend action. And the driver's job is to create the new frontend action. |
I am still strongly against making the filters part of the serialization process, we have learned that this is often a long-term mistake (e.g. doing decl merging in the AST reader). The filtering should be thought of as as AST -> AST transform and the AST serializer should just write what is being given to it. but, in either case this implies that the original AST is split into two paths - one going to the code-gen and one going to the BMI output; that is what this patch series is doing, I believe.
It is not one action since a BMI could be combined with any other code-gen action. Perhaps all that is needed is for the driver to add a Actually, since the FE can decide if a BMI is required from the source / AST there is actually no need to provide this (except, perhaps to disable it undeer some cases). |
We've already known that we don't have a good API to discard part of the AST from the experience in implementing discarding decls in GMF. Then we'd have to control it in the AST Writers.
No, it can be an action and we can control the usage of the BMI. |
Here is a draft of the patch series that allows for emitting both an object and a BMI from the same compiler invocation.
Because of point 1) below it's fairly limited in the command lines supported, you can do things like:
clang++ -std=c++20 foo.cpp -c -fmodule-file=X=some/dir/X,pcmwhich will generate
foo.oin theCWDandX.pcminCWD/some/dirNote that, like GCC's impl you no longer need to have a special name for module files, any C++ suffix should work (actually, I'm not 100% sure if the .cppm will work without some driver-side mods)
In terms of the mechanism to do the split, I'd hope it's pretty close.
Where more development will be required:
However, neither 1 or 2 should prevent this being a first step....
I"m interested in feedback on the impl.
@ChuanqiXu9 @dwblaikie @Bigcheese @mathstuf @boris-kolpackov @tahonermann