[PATCH] D38494: [SCEV] Handling for ICmp occuring in the evolution chain.

[Sanjoy Das via Phabricator via llvm-commits]

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

This is looking close to ready, just a few minor things inline.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:4092
+    BasicBlock *Latch = nullptr;
+    if (L && (Latch = L->getLoopLatch())) {
+      BranchInst *BI = dyn_cast<BranchInst>(Latch->getTerminator());
----------------
Doesn't look like `L` can be `nullptr` - `ScalarEvolution::createAddRecFromPHI` bails out early if `L` is `nullptr`.

I think you can rewrite this to be:

```
if (BasicBlock *Latch = ...) {
  ...
}
```



================
Comment at: lib/Analysis/ScalarEvolution.cpp:4115
+      case Instruction::Select: {
+        const SCEV *CondSE = visit(SE.getSCEV(I->getOperand(0)));
+        if (isa<SCEVConstant>(CondSE)) {
----------------
Calling `getSCEV` + `visit` seems a bit overkill -- can't you just compare `I->getOperand(0)` with `LoopBackOnPosCond` here?

How about adding an `Optional<bool> isValueKnown(Value *Cond)` function that returns `true` or `false` if `Cond` is equal to `LoopBackOnPosCond` else returns `None`, and using that function in both the cases?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4122
+        }
+      } break;
+      default: {
----------------
Typically LLVM style is would be putting the `break;` inside the curly brace block.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4137
+  // Set to true if loop back is on positive branch condition.
+  bool LoopBackOnPosCond;
+
----------------
A more idiomatic name for this is `BackedgeTakenOnTrue`.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:6518
 }
 
+
----------------
Whitespace damange?


================
Comment at: test/Analysis/ScalarEvolution/pr34538.ll:13
+  %cmp = icmp slt i32 %start.0, 10000
+  %inc = zext i1 %cmp to i32
+  %inc.start.0 = add nsw i32 %start.0, %inc
----------------
I'd put these two test cases in the same file with a somewhat more descriptive file name.  Perhaps you can put the PR number as the function name instead?


https://reviews.llvm.org/D38494