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

[Jatin Bhateja via Phabricator via llvm-commits]

jbhateja added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4149
+
+Optional<const SCEV *>
+SCEVBackedgeConditionFolder::compareWithBackedgeCondition(Value *IC) {
----------------
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.


================
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
+
----------------
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.  


Repository:
  rL LLVM

https://reviews.llvm.org/D38494