[PATCH] D25738: [IndVarSimplify] Use control-dependent range information to prove non-negativity

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:927
+    DefUserPair Key(Def, UseI);
+    ConstantRange Current(R.getBitWidth(), /*isFullSet=*/true);
+    auto It = PostIncRangeInfos.find(Key);
----------------
`Current` seems unused?


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:930
+    if (It == PostIncRangeInfos.end()) {
+      PostIncRangeInfos.insert(std::make_pair(Key, R));
+    } else {
----------------
You can use `PostIncRangeInfos.insert({Key, R});` here.

I'd also drop the braces.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1474
+    if (!NonNegativeDef) {
+      // We might have a control-dependent range information for this context
+      if (auto RangeInfo = getPostIncRangeInfo(NarrowDef, NarrowUser))
----------------
End comment with period.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1521
+  // Iterate over IV uses (including transitive ones) looking for IV increments
+  // in the form of 'add nsw %iv, <const>'. For each increment and each use of
+  // the increment calculate control-dependent range information basing on
----------------
s/in the form of/of the form/


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1594
+
+    ConstantRange CmpRHSRange = SE->getSignedRange(SE->getSCEV(CmpRHS));
+    ConstantRange CmpConstrainedLHSRange =
----------------
I'd s/`ConstantRange`/`auto` here.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1607
+       DTB = DTB->getIDom()) {
+    auto TI = DTB->getBlock()->getTerminator();
+
----------------
`auto *` here.

I'd actually just remove `TI` altogether -- and instead do:

```
auto *BI = dyn_cast<BranchInst>(DTB->getBlock()->getTerminator());
```



================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1618
+
+    UpdateRangeFromCondition(BI->getCondition());
+  }
----------------
Add a TODO here about also exploiting `false` edges in the future (feel free to implement it right now too -- it won't be that much more code).


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1623
+void WidenIV::calculatePostIncRanges(PHINode *OrigPhi) {
+  DenseSet<Instruction *> Visited;
+  SmallVector<Instruction *, 6> Worklist;
----------------
I'd perhaps use a `SmallPtrSet<16>` here.


================
Comment at: test/Transforms/IndVarSimplify/post-inc-range.ll:8
+; In order to do this indvars need to prove that the narrow IV def (%i.inc)
+; is not-negative from the range check inside of the loop.
+define void @test(i32* %base, i32 %limit, i32 %start) {
----------------
Can you please add a test case that checks that we do the right thing for a comparison with, say, `(%i * 3) + 1` where `%i` is the IV and there is a range check on `%i * 3`?  That is, check that we're actually using the worklist algorithm above.



https://reviews.llvm.org/D25738