[PATCH] D146776: [llvm] Preliminary fat-lto-objects support

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/include/llvm/Bitcode/EmbedBitcodePass.h:26
+  EmbedBitcodeOptions()
+      : EmbedBitcodeOptions(false, false, ModulePassManager()){};
+  EmbedBitcodeOptions(bool IsThinLTO, bool EmitLTOSummary,
----------------
extraneous `;`


================
Comment at: llvm/include/llvm/Bitcode/EmbedBitcodePass.h:28
+  EmbedBitcodeOptions(bool IsThinLTO, bool EmitLTOSummary,
+                      ModulePassManager &&MPM)
+      : IsThinLTO(IsThinLTO), EmitLTOSummary(EmitLTOSummary){};
----------------
this is never used


================
Comment at: llvm/include/llvm/Bitcode/EmbedBitcodePass.h:43
+      : EmbedBitcodePass(Opts.IsThinLTO, Opts.EmitLTOSummary,
+                         ModulePassManager()) {}
+  EmbedBitcodePass(bool IsThinLTO, bool EmitLTOSummary, ModulePassManager &&MPM)
----------------
if you really wanted to hook this up to the pipeline parsing (so you could specify a sub-passmanager) you'd have to thread through all the parsing code. probably not worth it


================
Comment at: llvm/include/llvm/Passes/PassBuilder.h:242
+  /// generated by running the passes from buildModuleSimplificationPipeline and
+  /// buildModuleOptimizationPipeline. This should result in object code very
+  /// close to the PerModuleDefaultPipeline, used when compiling without LTO.
----------------
comment out of date


================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:1480
 
+  // TODO: once D148010 lands, we can simplify this
+  MPM.addPass(EmbedBitcodePass(ThinLTO, EmitSummary,
----------------
this comment is obsolete now


================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:1266
+ModulePassManager PassBuilder::buildModuleOptimizationPipeline(
+    OptimizationLevel Level, ThinOrFullLTOPhase LTOPhase, bool IsFatLTO) {
   const bool LTOPreLink = isLTOPreLink(LTOPhase);
----------------
paulkirth wrote:
> aeubanks wrote:
> > do we still need `IsFatLTO` with the latest approach?
> That's a good point. I don't think so, so I can simplify this futher. Thanks for pointing this out.
still not removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146776