ychen added inline comments.
================
Comment at: llvm/lib/Passes/PassBuilder.cpp:1659
+ bool DebugLogging) {
+ for (auto &C : PipelineStartEPCallbacks)
+ C(MPM);
----------------
aeubanks wrote:
> 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.
> 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.
Definitely agree that this is in line with legacy PM. I don't feel strongly to
go either way. Since @echristo is actively tuning O0, it would be helpful to
know his opinion.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89158/new/
https://reviews.llvm.org/D89158
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
