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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth marked an inline comment as done.
paulkirth added a comment.

In D146776#4374005 <https://reviews.llvm.org/D146776#4374005>, @aeubanks wrote:

> These are all performance concerns for the non-LTO native objects, not correctness right? I think if we clearly document that performance might be very meh for fat LTO non-LTO native objects this would be fine.

In the prior version, I think its too hard to say if running pipelines, like `ModuleSimplification`, multiple times is safe/correct without some fairly detailed analysis. I can easily imagine any transform that is only expected to run once may introduce a subtle bug if run again on the same module. Nothing obvious here jumps out at me, but I'm also not confident enough to say that it can't happen.

That said, since FatLTO no longer use a modified/custom pipeline but uses the (Thin)LTO and Module piplines on independed modules, there should no longer be any correctness nor performance issues over a `normal` compilation for the non-LTO object code.



================
Comment at: llvm/include/llvm/Bitcode/EmbedBitcodePass.h:32
+  EmbedBitcodePass(bool IsThinLTO, bool EmitLTOSummary,
+                   ModulePassManager &&MPM = ModulePassManager())
+      : IsThinLTO(IsThinLTO), EmitLTOSummary(EmitLTOSummary),
----------------
aeubanks wrote:
> is this default param necessary?
IIRC it was required for some reason, but let me double check that.


================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:1266
+ModulePassManager PassBuilder::buildModuleOptimizationPipeline(
+    OptimizationLevel Level, ThinOrFullLTOPhase LTOPhase, bool IsFatLTO) {
   const bool LTOPreLink = isLTOPreLink(LTOPhase);
----------------
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.


================
Comment at: llvm/lib/Passes/PassRegistry.def:61
 MODULE_PASS("extract-blocks", BlockExtractorPass({}, false))
+MODULE_PASS("embed-bitcode", EmbedBitcodePass(true, true))
 MODULE_PASS("forceattrs", ForceFunctionAttrsPass())
----------------
aeubanks wrote:
> these should be parameterizable, see `MODULE_PASS_WITH_PARAMS`
That is a much better way to handle this. Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146776