[PATCH] New pass: inductive range check elimination

[Philip Reames]

As with Hal, I'm happy to see this evolve in tree, provided that checking it in doesn't break any existing use cases and that you're going to continue to evolve this in the near term.  Given I happen to know the later is true, LGTM.  Feel free to address comments in separate submissions.  (If you choose that option, document that's what you're planning on doing in the submission comment.)


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:85
@@ +84,3 @@
+  /// < Range.first, then the value denotes the empty range.
+  typedef std::pair<Value *, Value *> Range;
+  typedef SpecificBumpPtrAllocator<InductiveRangeCheck> AllocatorTy;
----------------
It feels like range is a first class concept here.  Maybe extract this out?  Probably only file local at the moment, but separate from IRC.  

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:158
@@ +157,3 @@
+
+  if (match(Condition, m_And(m_Value(A), m_Value(B)))) {
+    Value *IndexV = nullptr;
----------------
This structure of checks seems very fragile.  In particular, I would expect things like flatten cfg to create patterns this misses.  Also, that "sink load along path with uses" optimization we were talking to enable vectorization would also break this.

Putting a TODO here for the moment is fine, but I suspect we'll want something more robust quickly.  

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:164
@@ +163,3 @@
+        m_ICmpWithPred<ICmpInst::ICMP_SGT>(
+            m_Value(IndexV), m_ConstantInt<-1>()));
+
----------------
These lambdas looks very similar to code inside InstCombine.  Could they be extracted to some common place?

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:200
@@ +199,3 @@
+      Index = SE.getSCEV(A);
+      if (isa<SCEVCouldNotCompute>(Index) || !SE.isKnownNonNegative(Index))
+        return false;
----------------
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.

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

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

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

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:253
@@ +252,3 @@
+
+  InductiveRangeCheck *IRC = new (A.Allocate()) InductiveRangeCheck;
+  IRC->Length = Length;
----------------
This looks like a constructor begging to be born.

================
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);
----------------
This feels like it should be on IRBuilder.

At minimum, you should use IRBuilder so that these eagerly collapse.  

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:445
@@ +444,3 @@
+
+  BasicBlock *Latch = L->getLoopLatch();
+  if (!L->isLoopExiting(Latch)) {
----------------
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?

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

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

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

================
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);
----------------
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.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:796
@@ +795,3 @@
+  InductiveRangeCheck::AllocatorTy IRCAlloc;
+  SmallVector<InductiveRangeCheck *, 16> RangeChecks;
+  ScalarEvolution &SE = getAnalysis<ScalarEvolution>();
----------------
The fact these are non-owning pointers isn't particularly clear.  It might be better to structure this vector as containing IRCs not IRC*s.  

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

This might be a convenient helper function too.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:816
@@ +815,3 @@
+
+  BasicBlock *Preheader = L->getLoopPreheader();
+  if (!Preheader) {
----------------
Your legality checks should be before you do any work.

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

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:830
@@ +829,3 @@
+  for (InductiveRangeCheck *IRC : RangeChecks) {
+    auto Result = IRC->computeSafeIterationSpace(SE, ExprInsertPt);
+    if (Result.hasValue()) {
----------------
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.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:840
@@ +839,3 @@
+
+  if (!SafeIterRange.hasValue()) return false;
+
----------------
When would this trigger?

================
Comment at: test/Transforms/InductiveRangeCheckElimination/single.ll:3
@@ +2,3 @@
+
+define void @rce.0(i32 *%arr, i32 *%a_len_ptr, i32 %n) {
+; CHECK-LABEL: rce.0
----------------
You need far more tests to claim any kind of reasonable coverage here.  

In particular, trivial loop tests which check all of the corner-cases in your matching logic, both positive and negative.

http://reviews.llvm.org/D6693

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