[PATCH] D29620: [JumpThreading] Thread through guards

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp:2076
+    if (Parent == Pred2->getSinglePredecessor())
+      if (BranchInst *BI = dyn_cast<BranchInst>(Parent->getTerminator()))
+        for (auto &I : *BB)
----------------
mkazantsev wrote:
> sanjoy wrote:
> > sanjoy wrote:
> > > mkazantsev wrote:
> > > > sanjoy wrote:
> > > > > mkazantsev wrote:
> > > > > > anna wrote:
> > > > > > > Hi Max, I think you need to check that branch is a conditional branch. Otherwise we can fail while trying to get the condition for the branch in `ThreadGuard` at line 2091.
> > > > > > You're right, I need to add this check here.
> > > > > I guess this is possible when `Pred1` and `Pred2` are the same, like:
> > > > > 
> > > > > 
> > > > > ```
> > > > > Parent:
> > > > >   br label %pred
> > > > > 
> > > > > pred:
> > > > >   switch ... 4 -> BB; 5 -> BB; ...
> > > > > 
> > > > > BB:
> > > > >   ...
> > > > > ```
> > > > > 
> > > > > 
> > > > > Are there other bits in this patch that are wrong in that situation?
> > > > > 
> > > > I think another example could be
> > > > 
> > > >   Parent:
> > > >     br %cond Pred, Pred
> > > > 
> > > >   Pred:
> > > >     br BB
> > > > 
> > > >   BB
> > > >     ...
> > > > 
> > > > So I would rather add both checks "branch is conditional" and pred1 != pred2.
> > > I think `getSinglePredecessor` on `Pred` will return `nullptr` in that case.  `getUniquePredecessor` would have returned `Parent`.
> > Also, you would not have had two preds for `BB`.
> Agreed. However, in your example the discussed situation doesn't happen as well. The thing is that JumpTheading will merge Parent and pred blocks before it gets to BB and runs into that crash. I will try to construct a test that reproduces the problem.
> 
> Anna, do you have a simple test where this crash happens? I'd appreciate if you provided it.
I was able to create a test that looks like

  define i32 @not_a_diamond1(i32 %a, i1 %cond1) {
    br i1 %cond1, label %Pred, label %Exit
  Pred:
    switch i32 %a, label %Exit [
      i32 10, label %Merge
      i32 20, label %Merge
    ]
  Merge:
    call void(i1, ...) @llvm.experimental.guard( i1 %cond1 )[ "deopt"() ]
    br label %Exit
  Exit:
    ret i32 %a
  }

Despite the condition is conditional, here pred1 = pred2, and the test is buggy.


Repository:
  rL LLVM

https://reviews.llvm.org/D29620