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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers 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:
> > > 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88438