[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)
----------------
sanjoy wrote:
> mkazantsev wrote:
> > 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.
> It is easy to fool JumpThreading's block ordering though, to prevent it from collapsing blocks:
> 
> ```
> declare void @llvm.experimental.guard(i1, ...)
> 
> define void @f(i32 %a, i1 %c) {
> entry:
>   br label %parent
> 
> merge:
>   call void(i1, ...) @llvm.experimental.guard(i1 %c)[ "deopt"() ]
>   ret void
> 
> pred:
>   switch i32 %a, label %exit [
>     i32 10, label %merge
>     i32 20, label %merge
>   ]
> 
> parent:
>   br label %pred
> 
> exit:
>   ret void
> }
> ```
> 
> on `./bin/opt -jump-threading` with @anna 's revert reverted crashes as:
> 
> ```
> Assertion failed: (isConditional() && "Cannot get condition of an uncond branch!"), function getCondition, file ../../include/llvm/IR/Instructions.h, line 2989.
> 0  opt                      0x00000001044890bc llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60
> 1  opt                      0x0000000104489639 PrintStackTraceSignalHandler(void*) + 25
> 2  opt                      0x00000001044856a9 llvm::sys::RunSignalHandlers() + 425
> 3  opt                      0x0000000104489b72 SignalHandler(int) + 354
> 4  libsystem_platform.dylib 0x00007fffdfc3ebba _sigtramp + 26
> 5  libsystem_platform.dylib 000000000000000000 _sigtramp + 540808288
> 6  libsystem_c.dylib        0x00007fffdfac5420 abort + 129
> 7  libsystem_c.dylib        0x00007fffdfa8c893 basename_r + 0
> 8  opt                      0x0000000103e82db8 llvm::BranchInst::getCondition() const + 104
> 9  opt                      0x00000001040d4b39 llvm::JumpThreadingPass::ThreadGuard(llvm::BasicBlock*, llvm::IntrinsicInst*, llvm::BranchInst*) + 105
> 10 opt                      0x00000001040cd17f llvm::JumpThreadingPass::ProcessGuards(llvm::BasicBlock*) + 479
> 11 opt                      0x00000001040c8ecd llvm::JumpThreadingPass::ProcessBlock(llvm::BasicBlock*) + 541
> 12 opt                      0x00000001040c87a7 llvm::JumpThreadingPass::runImpl(llvm::Function&, llvm::TargetLibraryInfo*, llvm::LazyValueInfo*, bool, std::__1::unique_ptr<llvm::BlockFrequencyInfo, std::__1::default_delete<llvm::BlockFrequencyInfo> >, std::__1::unique_ptr<llvm::BranchProbabilityInfo, std::__1::default_delete<llvm::BranchProbabilityInfo> >) + 1959
> 13 opt                      0x00000001040d5e21 (anonymous namespace)::JumpThreading::runOnFunction(llvm::Function&) +2161
> ```
Thanks! The fix I've prepared passes both tests. I will add them to the test suite.


Repository:
  rL LLVM

https://reviews.llvm.org/D29620