[PATCH] D36215: [IRCE] Identify loops with latch comparison against current IV value

[Anna Thomas via Phabricator via llvm-commits]

anna requested changes to this revision.
anna added a comment.
This revision now requires changes to proceed.

Comments inline.



================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:464
   bool IsSignedPredicate;
+  bool IsIndVarNext;
 
----------------
Pls add a comment for what IndVarBase and IndVarNext means. It's pretty clear in the summary of the bug, but add it as a comment in the code as well.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:840
   bool IsSignedPredicate = true;
+  bool IsIndVarNext = false;
   ConstantInt *StepCI;
----------------
Pls add a comment that this checks that the comparison is made against iv.next.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:848
+  const SCEV *IndVarStart = nullptr;
+  if (isa<PHINode>(LeftValue) &&
+      cast<PHINode>(LeftValue)->getParent() == Header)
----------------
I'm not sure if this is enough to identify that it's from the current IV. 

What if we had some "noop" operations on that phi node, and the result of that operation was used in the comparison? Wouldn't we incorrectly mark as this is from iv.next, whereas it's infact from the current IV.
```
header:
iv = phi [0, entry] [iv.next, latch]
...

latch:
iv.trunc =  trunc iv to 16
iv.next = iv + 1
%cmp = slt iv.trunc, 100
br %cmp, header, exit
```
It could be bitcast or trunc or any other such operations. I think we need to "look through these operations" rather than directly comparing against a phi node.

Please add test cases as well.



https://reviews.llvm.org/D36215