[PATCH] D109762: [NewPM][SimpleLoopUnswitch] Add DivergenceInfo

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2803
+    Function *F = L.getHeader()->getParent();
+    PostDominatorTree PDT(*F);
+    DivergenceInfo DI(*F, DT, PDT, LI, TTI, /*KnownReducible*/ true);
----------------
bcahoon wrote:
> aeubanks wrote:
> > sameerds wrote:
> > > sameerds wrote:
> > > > bcahoon wrote:
> > > > > aeubanks wrote:
> > > > > > bcahoon wrote:
> > > > > > > aeubanks wrote:
> > > > > > > > bcahoon wrote:
> > > > > > > > > arsenm wrote:
> > > > > > > > > > Constructing your own PostDominatorTree seems not good. Should this be a pass dependency for divergent targets?
> > > > > > > > > I agree that it's not good, but I believe the only way to do this as a dependency/required is to have all the loop passes update the post-dominator tree info when needed (which is what's done for dominators and several other loop analyses).
> > > > > > > > See the loop analyses part of https://llvm.org/docs/NewPassManager.html#using-analyses for limitations on accessing function analyses in a loop pass.
> > > > > > > Thanks for that reference.  If I understand correctly, another approach to adding divergence analysis would be to add an entry to LoopStandardAnalysisResults for DivergenceAnalysisInfo (but, at least initially, without the guarantee that loop passes update it).  That way it can be accessed by SimpleLoopUnswtich.
> > > > > > > 
> > > > > > > Then, in PassBuilder.cpp add an instance of the DivergeranceAnalysis prior to SimpleLoopUnswitch. This also requires a call to createFunctionToLoopPassAdaptor between DivergenceAnalysis and SimpleLoopUnswitch.  We could do this only for targets that have branch divergence so that the pass pipeline remains the same for other targets.
> > > > > > > 
> > > > > > > This approach seems most similar to how the legacy pass manager works. My main hesitation is adding DivergenceAnalysis to LoopStandardAnalysisResults.
> > > > > > I don't think splitting up the loop pipeline to add in a function pass (to request a function analysis) is ok
> > > > > > also, adding DivergenceAnalysis to LoopStandardAnalysisResults doesn't seem right since expecting all loop passes to update DivergenceAnalysis doesn't seem right
> > > > > > 
> > > > > > I think the proper way is to relax the constraint that a loop pass can't request a function analysis on demand. Part of the reason we don't allow this for other IR units is potential future concurrency and determinism, but I don't think we will ever do concurrency for loop passes (as in run loop passes on multiple loops in a function concurrently). I believe @asbirlea has mentioned something like this in the past, any thoughts?
> > > > > > 
> > > > > > Just curious, are you actually seeing benchmarks where this nontrivial loop unswitching matters?
> > > > > > Just curious, are you actually seeing benchmarks where this nontrivial loop unswitching matters?
> > > > > 
> > > > > Yes, I've been tracking down the source of some performance regressions that we're seeing after the switch from the legacy pass manager to the new pass manager.  I've seen a couple instances where non-trivial loop unswitching does improve performance (though, in one instance, I've noticed that the threshold needs to increase in simple loop unswitch to recover performance). 
> > > > Just to add some detail, here's one idea of "splitting 
> > > > I don't think splitting up the loop pipeline to add in a function pass (to request a function analysis) is ok
> > > > also, adding DivergenceAnalysis to LoopStandardAnalysisResults doesn't seem right since expecting all loop passes to update DivergenceAnalysis doesn't seem right
> > > > 
> > > > I think the proper way is to relax the constraint that a loop pass can't request a function analysis on demand.
> > > 
> > > Just FYI, here's one idea of how "splitting the loop pipeline" might look like if we attempted that:
> > > 
> > > https://lists.llvm.org/pipermail/llvm-dev/2021-February/148619.html
> > I'm currently talking to some people to see if we can do some redesigning and allow loop passes to request function analyses on demand.
> Great to hear. Let me know if there is anything I can do to help. Do you think it's worthwhile to merge this patch now or wait until the outcome of the redesign?
Merging this now is fine. (and still LGTM)


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

https://reviews.llvm.org/D109762