[PATCH] D88438: BreakCriticalEdges: do not split the critical edge from a CallBr indirect successor

Arthur Eubanks via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 15 11:14:39 PST 2021


aeubanks added inline comments.


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/callbr-critical-edge-splitting.ll:1
+; RUN: opt < %s -o /dev/null -loop-reduce
+
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > aeubanks wrote:
> > > nickdesaulniers wrote:
> > > > nickdesaulniers wrote:
> > > > > nickdesaulniers wrote:
> > > > > > nickdesaulniers wrote:
> > > > > > > nickdesaulniers wrote:
> > > > > > > > nickdesaulniers wrote:
> > > > > > > > > nickdesaulniers wrote:
> > > > > > > > > > nickdesaulniers wrote:
> > > > > > > > > > > MaskRay wrote:
> > > > > > > > > > > > Just use the new PM way: `-passes='loop(loop-reduce)'`
> > > > > > > > > > > If I do that for both tests:
> > > > > > > > > > > 1. this one passes before this patch has been applied. (bad)(lol, wut)
> > > > > > > > > > > 2. the other test still fails. (good)
> > > > > > > > > > > 
> > > > > > > > > > > Is there something funny with NPM that I need to specify additional passes in addition to `loop(loop-reduce)`?
> > > > > > > > > > Let me be more precise:
> > > > > > > > > > 
> > > > > > > > > > Before this patch is applied:
> > > > > > > > > > 
> > > > > > > > > > -loop-reduce:
> > > > > > > > > >   callbr-critical-edge-splitting.ll:  fail
> > > > > > > > > >   callbr-critical-edge-splitting2.ll: fail
> > > > > > > > > > 
> > > > > > > > > > -passes=loop-reduce:
> > > > > > > > > >   callbr-critical-edge-splitting.ll:  pass (WTF)
> > > > > > > > > >   callbr-critical-edge-splitting2.ll: fail
> > > > > > > > > > 
> > > > > > > > > > -passes='loop(loop-reduce)':
> > > > > > > > > >   callbr-critical-edge-splitting.ll:  pass (WTF)
> > > > > > > > > >   callbr-critical-edge-splitting2.ll: fail
> > > > > > > > > > 
> > > > > > > > > > So I guess it makes sense to drop `-passes=loop-reduce` if that's covered by `-passes='loop(loop-reduce)'`, but it's curious to me from a TDD perspective why the test is green pre-patch for one pass manager (the new one) and red pre-patch for another pass manager (the old one).
> > > > > > > > > > 
> > > > > > > > > > > The legacy PM test does not add too much value for new tests as we'll soon make a switch.
> > > > > > > > > > 
> > > > > > > > > > But the test is red pre-patch for legacy PM. WTF
> > > > > > > > > The output between -debug-only=loop-pass is a bit different between `-loop-reduce` and `-passes='loop(loop-reduce)'` as well...
> > > > > > > > Interesting, ok, so `-print-after-all` shows that `-loop-reduce` (ie. old PM) runs more passes than just `-loop-reduce`.  Looks like `-loop-reduce` additionally runs:
> > > > > > > > 
> > > > > > > > 1. -loop-simplify
> > > > > > > > 2. -loop-reduce
> > > > > > > > 3. -verify
> > > > > > > > 
> > > > > > > > I don't know how to run `-loop-simplify` with NPM (guessed: `-passes='loop(loop-simplify,loop-reduce)'`; doesn't work), but if I did I guess that might also make `callbr-critical-edge-splitting2.ll` pass pre-patch.
> > > > > > > > 
> > > > > > > > In that case, unless it's possible to run `-loop-simplify` with NPM, then I think I will simply drop the OPM run from `callbr-critical-edge-splitting2.ll`, since it doesn't add any signal, but keep OPM for `callbr-critical-edge-splitting.ll` since it does. Thoughts?
> > > > > > > Aha, `-passes='loop-simplify,loop(loop-reduce)'`!
> > > > > > I would have expected `-passes='loop-simplify,loop(loop-reduce)'` (new pass manager) to fail pre-patch similar to `-lood-reduce` (old pass manager).  It does not.  I'm out of ideas here.  Anyone know any tips or reasons why we get different results between new and old pass manager for loop reduction?
> > > > > > similar to -lood-reduce
> > > > > 
> > > > > similar to `-loop-reduce` (I don't have it misspelled locally or copy+paste mistakes)
> > > > > 
> > > > > Otherwise, I'm thinking of committing this as is.
> > > > @aeubanks I saw https://lists.llvm.org/pipermail/llvm-dev/2021-January/147695.html / https://bugs.llvm.org/show_bug.cgi?id=46649; is this thread of questions about different behavior between OPM and NPM something we should worry about?
> > > The new PM will run LCSSA and LoopSimplify before any loop passes: https://github.com/llvm/llvm-project/blob/6227069bdce6b0c3c22f0a0c8f1aef705985125a/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h#L415.
> > > 
> > > The legacy PM does not by default, but the loop-reduce legacy pass depends on loop-simplify. Adding `-loop-simplify -lcssa` at the beginning will probably make both RUN lines behave the same. Or just you can make the input already in LCSSA form.
> > I tried `-passes='loop-simplify,lcssa,loop(loop-reduce)'` (and `-passes='loop-simplify,lcssa,loop-reduce'`), but `callbr-critical-edge-splitting.ll` still seems to pass before this patch is applied, which is unexpected.  So there's still something different here between pass managers.  (Also, I'm surprised that `-print-after-all` and `-print-before-all` don't seem to mention anything related to `lcssa` for LPM/OPM.)
> > 
> > Ok, this is weird: comparing the output of `-print-before-all` between `-loop-reduce` and `-passes='loop-simplify,loop(loop-reduce)'`:
> > 
> > `-loop-reduce`:
> > ```
> > *** IR Dump Before Canonicalize natural loops ***
> > *** IR Dump Before Loop Strength Reduction ***
> > ```
> > 
> > `-passes='loop-simplify,loop(loop-reduce)'`:
> > ```
> > *** IR Dump Before VerifierPass ***
> > *** IR Dump Before LoopSimplifyPass ***
> > *** IR Dump Before LoopSimplifyPass ***
> > *** IR Dump Before LCSSAPass ***
> > *** IR Dump Before LoopStrengthReducePass ***
> > *** IR Dump Before LoopStrengthReducePass ***
> > *** IR Dump Before VerifierPass ***
> > *** IR Dump Before PrintModulePass ***
> > ```
> > 
> > `Canonicalize natural loops` looks to me like `loop-simplify`, and yet when I explicitly run `loop-simplify` via NPM, the printed pass name is `LoopSimplifyPass` (I would have expected `Canonicalize natural loops`).  What is going on?
> llvm-project/llvm/lib/Transforms/Utils/LoopSimplify.cpp has the following:
> 
> ```
> INITIALIZE_PASS_BEGIN(LoopSimplify, "loop-simplify",
>                 "Canonicalize natural loops", false, false)
> INITIALIZE_PASS_DEPENDENCY(AssumptionCacheTracker)
> INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
> INITIALIZE_PASS_DEPENDENCY(LoopInfoWrapperPass)
> INITIALIZE_PASS_END(LoopSimplify, "loop-simplify",
>                 "Canonicalize natural loops", false, false)
> ```
> so it looks like `-loop-simplify` is silently running 3 additional passes? (That's annoying that those passes don't print anything via -print-before-all, unless they're not being run). Looking at DominatorTreeWrapperPass, it looks like that's not ported to run with NPM? Same for LoopInfoWrapperPass.
> 
> At this point; I'm just going to commit this test invoking both pass managers: this is a bug visible with LPM, but it should not regress for NPM.
`Canonicalize natural loops` = `LoopSimplifyPass`. The naming is different for implementation reasons.

`AssumptionCacheTracker`/`DomTree`/`LoopInfo` are analysis passes that the pass uses, they won't show up in `--print-before/after-all`. The new PM has a different way of querying analyses.

The legacy PM doesn't automatically run LCSSA, so you won't see it unless the pass depends on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88438



More information about the llvm-commits mailing list