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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4149
+
+Optional<const SCEV *>
+SCEVBackedgeConditionFolder::compareWithBackedgeCondition(Value *IC) {
----------------
jbhateja wrote:
> sanjoy wrote:
> > sanjoy wrote:
> > > I think it is better to return `Optional<bool>` here, and instead call `getOne` and `getZero` in the `default:` case in the switch on `I->getOpcode()` which is the only place that needs it.  It will also make the `case Instruction::Select:` code a bit simpler.
> > This wasn't addressed?
> My bad I missed it, 
> BTW I'm not clear how you want this and if current code is not clean.
I was suggesting:

```
Optional<bool> compareWithBackedgeCondition(Value *IC) {
  return BackedgeCond == IC ? IsPositiveBECond : None;
}
```

and then putting the `getZero()` vs. `getOne()` in the `default:` case.  In the `Select` case you would not have to call `getZero()` and `getOne()`


================
Comment at: test/Analysis/ScalarEvolution/pr34538.ll:32
+  %inc.start.0 = add nsw i32 %start.0, %select_ext
+  br i1 %cmp, label %do.body, label %do.end
+
----------------
jbhateja wrote:
> sanjoy wrote:
> > jbhateja wrote:
> > > sanjoy wrote:
> > > > Please also add a test case where both the latch branches branch to the loop header.
> > > This will break the latch detection at header as getLoopLatch() looks at all the incoming branches  and if sees more than one incoming branch from a block within loop then it returns null. Though two disjoint natural-loops can have same headers and two loop latches are possible.  Adding a test will be futile here.
> > > 
> > > 
> > I see, that's a very good point.  That means the `BI->getSuccessor(0) != BI->getSuccessor(1)` can be an assert instead?
> Exaclty, Is post commit NFC for this and your proposed code ok here.  
Yes, a post-commit NFC fixup is fine (feel free to push without review).


Repository:
  rL LLVM

https://reviews.llvm.org/D38494