[SimplifyCFG] Constant fold a branch implied by it's incoming edge

[reames]

Summary

The most common use case is when eliminating redundant range checks in an example like the following:
c = a[i+1] + a[i];

For the moment, the implies function only handles a single simple case. I plan to extend this function to handle more complex examples, but I wanted to separate a single transform based on it to make review easy. I'll also be implementing a profile guided version targeted at removing range checks in (a[i] + a[i+1]) as well. That will follow in a later patch.

In terms of code placement/testing, do folks think it would make sense to implement the implies function in InstructionSimplify? You can phrase an implication question as icmpa <= icmpb. Having it in InstSimpify would make it easier to test, but I'm not sure we actually want to start handling complex implications in InstSimplify. Thoughts?

[sanjoy]

I think it makes sense to put this in InstructionSimplify for now. If it gets too large we can think of moving it into its own LogicUtils.h file.

lib/Transforms/Utils/SimplifyCFG.cpp

2345

Why not use matchers from PatternMatch.h here? I think that will make the logic a lot more obvious.

[reames]

I moved the implication logic into SimplifyInstruction and separated that into it's own review (http://reviews.llvm.org/D13074). Once that has gone in, I'll update this review.

[reames]

Rebase. Ping. Sanjoy?

[reames]

Fix a minor merge problem I hadn't noticed in the last diff.

[sanjoy]

lgtm modulo minor nits

lib/Transforms/Utils/SimplifyCFG.cpp

2425

I don't think you need to check PBI->getSuccessor(0) != PBI->getSuccessor(1) -- please verify this, but I think getSinglePredecessor (as opposed to getUniquePredecessor) should return nullptr if you have two branches from the same block to BB.

2428

Since you're not creating vectors of i1s, I think ConstantInt::getTrue(BB->getContext()) should be sufficient.

[reames]

lib/Transforms/Utils/SimplifyCFG.cpp

2425

After reading the code, I wasn't sure about this. I saw nothing which made it obvious, so I left the code as is.

2428

Fixed.

lib/Transforms/Utils/SimplifyCFG.cpp

for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E; ++I)
       if (isa<DbgInfoIntrinsic>(*I))
         I->clone()->insertBefore(PBI);
 
     return true;
   }
   return false;
 }
 
+/// Return true if B is known to be implied by A
+static bool implies(Value *A, Value *B) {
+  // TODO: Consider extending this to vector of i1?
+  assert(A->getType()->isIntegerTy(1) && B->getType()->isIntegerTy(1));
+  
+  // A ==> A by definition
+  if (A == B) return true;
+
+  // i +_{nsw} C_{>0} <s L ==> i <s L
+  auto *ICmpA = dyn_cast<ICmpInst>(A);
+  auto *ICmpB = dyn_cast<ICmpInst>(B);
+  if (!ICmpA || !ICmpB ||
+      ICmpA->getPredicate() != ICmpInst::ICMP_SLT ||
+      ICmpB->getPredicate() != ICmpInst::ICMP_SLT ||
+      ICmpA->getOperand(1) != ICmpB->getOperand(1))
+    return false;
+
+  auto *BO = dyn_cast<OverflowingBinaryOperator>(ICmpA->getOperand(0));
+  if (!BO || !BO->hasNoSignedWrap() ||
+      BO->getOperand(0) != ICmpB->getOperand(0))
+    return false;
+
+  ConstantInt *C = dyn_cast<ConstantInt>(BO->getOperand(1));
+  if (!C || C->isNegative())
+    return false;
+  
+  return true;    
+}
+
 /// If we have a conditional branch as a predecessor of another block,
 /// this function tries to simplify it.  We know
 /// that PBI and BI are both conditional branches, and BI is in one of the
 /// successor blocks of PBI - PBI branches to BI.
 static bool SimplifyCondBranchToCondBranch(BranchInst *PBI, BranchInst *BI) {
   assert(PBI->isConditional() && BI->isConditional());
   BasicBlock *BB = BI->getParent();
 
if (BlockIsSimpleEnoughToThreadThrough(BB)) {
         }
       }
 
       BI->setCondition(NewPN);
       return true;
     }
   }
 
+  // If BI is reached from the true path of PBI and PBI's condition implies
+  // BI's condition, we know the direction of the BI branch.
+  if (PBI->getSuccessor(0) == BI->getParent() &&
+      implies(PBI->getCondition(), BI->getCondition()) &&
+      PBI->getSuccessor(0) != PBI->getSuccessor(1) &&
+      BB->getSinglePredecessor()) {
+    // Turn this into a branch on constant.
+    auto *OldCond = BI->getCondition();
+    BI->setCondition(ConstantInt::getTrue(Type::getInt1Ty(BB->getContext())));
+    RecursivelyDeleteTriviallyDeadInstructions(OldCond);
+    return true;  // Nuke the branch on constant.
+  }
+
   // If this is a conditional branch in an empty block, and if any
   // predecessors are a conditional branch to one of our destinations,
   // fold the conditions into logical ops and one cond br.
   BasicBlock::iterator BBI = BB->begin();
   // Ignore dbg intrinsics.
   while (isa<DbgInfoIntrinsic>(BBI))
     ++BBI;
   if (&*BBI != BI)

test/Transforms/SimplifyCFG/implied-cond.ll

+; RUN: opt %s -S -simplifycfg | FileCheck %s
+; Check for when one branch implies the value of a successors conditional and
+; it's not simply the same conditional repeated.
+
+define void @test(i32 %length.i, i32 %i) {
+; CHECK-LABEL: @test
+  %iplus1 = add nsw i32 %i, 1
+  %var29 = icmp slt i32 %iplus1, %length.i
+; CHECK: br i1 %var29, label %in_bounds, label %out_of_bounds
+  br i1 %var29, label %next, label %out_of_bounds
+
+next:
+; CHECK-LABEL: in_bounds:
+; CHECK-NEXT: ret void
+  %var30 = icmp slt i32 %i, %length.i
+  br i1 %var30, label %in_bounds, label %out_of_bounds2
+
+in_bounds:
+  ret void
+
+out_of_bounds:
+  call void @foo(i64 0)
+  unreachable
+
+out_of_bounds2:
+  call void @foo(i64 1)
+  unreachable
+}
+
+; If the add is not nsw, it's not safe to use the fact about i+1 to imply the
+; i condition since it could have overflowed.
+define void @test_neg(i32 %length.i, i32 %i) {
+; CHECK-LABEL: @test_neg
+  %iplus1 = add i32 %i, 1
+  %var29 = icmp slt i32 %iplus1, %length.i
+; CHECK: br i1 %var29, label %next, label %out_of_bounds
+  br i1 %var29, label %next, label %out_of_bounds
+
+next:
+  %var30 = icmp slt i32 %i, %length.i
+; CHECK: br i1 %var30, label %in_bounds, label %out_of_bounds2
+  br i1 %var30, label %in_bounds, label %out_of_bounds2
+
+in_bounds:
+  ret void
+
+out_of_bounds:
+  call void @foo(i64 0)
+  unreachable
+
+out_of_bounds2:
+  call void @foo(i64 1)
+  unreachable
+}
+
+
+define void @test2(i32 %length.i, i32 %i) {
+; CHECK-LABEL: @test2
+  %iplus100 = add nsw i32 %i, 100
+  %var29 = icmp slt i32 %iplus100, %length.i
+; CHECK: br i1 %var29, label %in_bounds, label %out_of_bounds
+  br i1 %var29, label %next, label %out_of_bounds
+
+next:
+  %var30 = icmp slt i32 %i, %length.i
+  br i1 %var30, label %in_bounds, label %out_of_bounds2
+
+in_bounds:
+  ret void
+
+out_of_bounds:
+  call void @foo(i64 0)
+  unreachable
+
+out_of_bounds2:
+  call void @foo(i64 1)
+  unreachable
+}
+
+declare void @foo(i64)
+