[PATCH] D89158: [NewPM] Run all EP callbacks under -O0

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:1659
+                                             bool DebugLogging) {
+  for (auto &C : PipelineStartEPCallbacks)
+    C(MPM);
----------------
ychen wrote:
> asbirlea wrote:
> > aeubanks wrote:
> > > ychen wrote:
> > > > What I have in mind is a newly added `O0EPCallbacks` field in PassBuilder class. Then we can keep existing EPCallbacks (including PipelineStartEPCallbacks) for >O0 optimization pipeline. Yeah, then you need to add related passes to O0EPCallbacks (for BPF in this case).
> > > It's a tradeoff between having to specify required passes in both O0EPCallbacks and PipelineStartEPCallbacks which is repetitive, versus making all callbacks in PipelineStartEPCallbacks run at -O0, meaning even optional passes in PipelineStartEPCallbacks will run at -O0 (may be skipped via optnone).
> > > 
> > > The legacy PM chooses the first, and I'm inclined to keep it that way just for consistency.
> > > 
> > > If we did go down the second route, we could just have a second TargetMachine API like TargetMachine::addO0Passes() which directly adds passes to a ModulePassManager.
> > This seems more in line with the LPM behavior for O0.
> > If BPF needs those passes even for O0 they should be added as such.
> > It's a tradeoff between having to specify required passes in both O0EPCallbacks and PipelineStartEPCallbacks which is repetitive, versus making all callbacks in PipelineStartEPCallbacks run at -O0, meaning even optional passes in PipelineStartEPCallbacks will run at -O0 (may be skipped via optnone).
> Indeed.
> > The legacy PM chooses the first, and I'm inclined to keep it that way just for consistency.
> I'm lost here. Do you mean to say the second?
> > If we did go down the second route, we could just have a second TargetMachine API like TargetMachine::addO0Passes() which directly adds passes to a ModulePassManager.
> Do you mean to say the first? This is not much different from adding to O0EPCallbacks in  TargetMachine::registerPassBuilderCallbacks.
> 
> My proposal to have both O0EPCallbacks and PipelineStartEPCallbacks is that I'm not sure why we want to run all EP callbacks at O0. Do we have use cases for that?
> 
I am so sorry, yes I got them flipped.

As of the current patch, we're not running all callbacks, just PipelineStartEPCallbacks which is in line with the legacy PM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89158