[PATCH] New pass: inductive range check elimination

[Sanjoy Das]

I've replied to some of the comments inline.  I will upload a new version of the patch by the end of this week.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:200
@@ +199,3 @@
+      Index = SE.getSCEV(A);
+      if (isa<SCEVCouldNotCompute>(Index) || !SE.isKnownNonNegative(Index))
+        return false;
----------------
reames wrote:
> This doesn't look right.  There's nothing necessarily involving the induction variable here is there?
> 
> Actually, there's a broader problem in this entire function.  There's nothing that requires IndexV to actually be the induction variable of the loop.  
> 
> p.s. The upper range check with non-negative index should apply to the first case first.  You should factor this code.
`SplitRangeCheckCondition` does not interpret `Index` as anything related to the loop's induction variable, that happens later.  I'll add a comment making that invariant explicit.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:222
@@ +221,3 @@
+
+  if (SE.getLoopDisposition(UpperLimitSCEV, L) != ScalarEvolution::LoopInvariant) {
+    DEBUG(
----------------
reames wrote:
> The function says 'get', but the comment says 'make'?  
Fixed, thanks!

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:237
@@ +236,3 @@
+
+  if (BI->isUnconditional() || BI->getParent() == L->getLoopLatch())
+    return nullptr;
----------------
reames wrote:
> This seems restrictive (i.e. only loops in simplified form).  At minimum, add a TODO.  
This pass has `AU.addRequiredID(LoopSimplifyID)`

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:250
@@ +249,3 @@
+
+  if (!(IndexAddRec && IndexAddRec->getLoop() == L && IndexAddRec->isAffine()))
+    return nullptr;
----------------
reames wrote:
> This would be clearer as if (!x || !y || !z)
Changed this to

    bool IsAffineIndex =
      IndexAddRec && (IndexAddRec->getLoop() == L) && IndexAddRec->isAffine();
  
    if (!IsAffineIndex)
      return nullptr;


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:253
@@ +252,3 @@
+
+  InductiveRangeCheck *IRC = new (A.Allocate()) InductiveRangeCheck;
+  IRC->Length = Length;
----------------
reames wrote:
> This looks like a constructor begging to be born.
I like the current pattern better -- with a constructor it is easy to pass `Length` instead of `Offset` for example, since they're all `llvm::Value`s.  This pattern makes it clearer what is initialized with what.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:262
@@ +261,3 @@
+
+static Value *ConstructSMinOf(Value *A, Value *B, Instruction *InsertPt) {
+  ICmpInst *Cmp = new ICmpInst(InsertPt, ICmpInst::ICMP_SLT, A, B);
----------------
reames wrote:
> This feels like it should be on IRBuilder.
> 
> At minimum, you should use IRBuilder so that these eagerly collapse.  
Fixed, thanks!

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:445
@@ +444,3 @@
+
+  BasicBlock *Latch = L->getLoopLatch();
+  if (!L->isLoopExiting(Latch)) {
----------------
reames wrote:
> This seems very very limiting.  In particular, it would seem to exclude allmost all interesting cases.  Is this intentional?  
> 
> Just to make sure I have my terminology right: this is checking for a conditional branch which either goes to the header or exits the loop right?
I think loop rotation tries to canonicalize loops into this form (i.e. the loop's latch is a conditional exit).  Without this, it is difficult to split the loop's iteration space with low overhead -- there will need to be an extra check in the previously unconditional jump back to the loop's header from the latch which may or may not combine with checks in previous loop exits.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:451
@@ +450,3 @@
+
+  PHINode *CIV = L->getCanonicalInductionVariable();
+  if (!CIV) {
----------------
reames wrote:
> I didn't bother to read past here in this function.  This code needs broken up before it's reviewable.  
Agreed, this will be fixed in the next patch I upload for review.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:774
@@ +773,3 @@
+
+  Value *Begin = BinaryOperator::CreateNeg(OffsetV, "", Loc);
+  Value *End = BinaryOperator::CreateSub(getLength(), OffsetV, "", Loc);
----------------
reames wrote:
> You should use IRBuilder here so that these are eagerly simplified in trivial cases.
Agreed, will do.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:786
@@ +785,3 @@
+  if (!R1.hasValue()) return R2;
+  auto &R1Value = R1.getValue();
+
----------------
reames wrote:
> Is there any guarantee that R2 has a value?
`R2` is not an `Optional<>`

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:788
@@ +787,3 @@
+
+  Value *NewMin = ConstructSMaxOf(R1Value.first, R2.first, InsertPoint);
+  Value *NewMax = ConstructSMinOf(R1Value.second, R2.second, InsertPoint);
----------------
reames wrote:
> Given you're computing a recursive tree of mins, it might be worth talking about whether we can either a) balance the tree, or b) easy to optimize representation.  I worry about the depth of these select trees.
Agreed, but that is not the lowest hanging fruit at the moment. :)

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:799
@@ +798,3 @@
+
+  for (auto BBI : L->getBlocks()) {
+    if (BranchInst *TBI = dyn_cast<BranchInst>(BBI->getTerminator())) {
----------------
reames wrote:
> Add a comment along the lines of: //identify any range checks we can handle.
> 
> This might be a convenient helper function too.
Will do.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:816
@@ +815,3 @@
+
+  BasicBlock *Preheader = L->getLoopPreheader();
+  if (!Preheader) {
----------------
reames wrote:
> Your legality checks should be before you do any work.
Right, this is an obvious candidate for an early check.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:826
@@ +825,3 @@
+#ifndef NDEBUG
+  SmallVector<InductiveRangeCheck *, 4> RangeChecksToEliminate;
+#endif
----------------
reames wrote:
> I think this is dead code?
No, `RangeChecksToEliminate` is used later, also under `#ifndef NDEBUG`

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:830
@@ +829,3 @@
+  for (InductiveRangeCheck *IRC : RangeChecks) {
+    auto Result = IRC->computeSafeIterationSpace(SE, ExprInsertPt);
+    if (Result.hasValue()) {
----------------
reames wrote:
> It feels odd to me that your doing this for all range checks at once.  Is there an argument for why this is sufficient/desirable?  If so, comment.
I think if I do it for one array index at a time, I will end up with `N` pre / post loops.  Do you have something else in mind?

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:840
@@ +839,3 @@
+
+  if (!SafeIterRange.hasValue()) return false;
+
----------------
reames wrote:
> When would this trigger?
When we could not solve *any* of the range checks.  e.g

   for (i = 0 to N) {
     a[i * i] = ..
   }

http://reviews.llvm.org/D6693

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/