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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth marked 5 inline comments as done.
paulkirth added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:1489
+      buildModuleSimplificationPipeline(Level, ThinOrFullLTOPhase::None));
+  MPM.addPass(buildModuleOptimizationPipeline(Level, ThinOrFullLTOPhase::None,
+                                              /*IsFatLto*/ true));
----------------
tejohnson wrote:
> paulkirth wrote:
> > tejohnson wrote:
> > > Why pass in phase None for these calls to module simplification and optimization passes, instead of the corresponding post-LTO link phase? I would think we would want the complement of the phase provided earlier. I'm pretty sure there are some passes that are configured to run in exactly one of these phases, and we might end up duplicating them if running a pre-LTO link phase + a default (non-LTO) phase optimization pipeline.
> > The intent there was to closely mimic the non-lto pipeline. For now I've attempted to just split everything, but I'm not sure I love the approach. It's certainly simple, but I didn't like giving a pass its own passmanager... plus its does seem to be a bit wasteful to run two complete pipelines on the module.
> I don't think the prior code mimic'ed the non-LTO pipeline, however. My understanding is that the prior version did the following for the non embedded code on which it would generate a native object:
> 
> (Thin)LTOPreLink + ModuleSimplification(ThinOrFullLTOPhase::None) + ModuleOptimization(ThinOrFullLTOPhase::None)
> 
> So you would have been essentially duplicating ModuleSimplification (since it is invoked in both the *LTO pre-link and again with LTO Phase = None). That can cause some issues because there are passes that are only meant to be invoked once that you could duplicate. A good example is PGO instrumentation/annotation (see the conditions under which addPGOInstrPasses is called from buildModuleSimplificationPipeline).
> 
> With your new version, my understanding is that you are calling:
> 1. buildPerModuleDefaultPipeline on original Module -> generate native object
> 2. (Thin)LTOPreLink on Module clone -> embed IR
> 
> And buildPerModuleDefaultPipeline is largely just ModuleSimplification + ModuleOptimization. So I'm not sure about why it would be more wasteful (other than cloning the Module) - you are not running more pipelines total than in the prior code. Unless I am misunderstanding the former or current code, which is possible!
> I don't think the prior code mimic'ed the non-LTO pipeline, however. My understanding is that the prior version did the following for the non embedded code on which it would generate a native object:
> 
> (Thin)LTOPreLink + ModuleSimplification(ThinOrFullLTOPhase::None) + ModuleOptimization(ThinOrFullLTOPhase::None)
> 
> So you would have been essentially duplicating ModuleSimplification (since it is invoked in both the *LTO pre-link and again with LTO Phase = None). That can cause some issues because there are passes that are only meant to be invoked once that you could duplicate. A good example is PGO instrumentation/annotation (see the conditions under which addPGOInstrPasses is called from buildModuleSimplificationPipeline).
> 
> With your new version, my understanding is that you are calling:
> 1. buildPerModuleDefaultPipeline on original Module -> generate native object
> 2. (Thin)LTOPreLink on Module clone -> embed IR
> 
> And buildPerModuleDefaultPipeline is largely just ModuleSimplification + ModuleOptimization. So I'm not sure about why it would be more wasteful (other than cloning the Module) - you are not running more pipelines total than in the prior code. Unless I am misunderstanding the former or current code, which is possible!

I think that's an accurate summary. While my intent originally was to closely mimic the default pipeline, I don't think it was very close in reality. Also, as you pointed out the new approach isn't nearly as wasteful as I felt initially.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146776