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

[Arthur Eubanks via Phabricator via llvm-commits]

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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88438