[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:4108
+    const SCEV *Result = Expr;
+    bool InvariantF = SE.isLoopInvariant(Expr, L);
+
----------------
jbhateja wrote:
> sanjoy wrote:
> > No need to change it in this patch, but this constraint isn't necessary, is it?
> Not sure, why this constraint not necssary, why do we want to cross loop boundary  to explore evolution expression. If Expression is loop invariant either its a constant or its def lies outside the loop.
Yes, it does not help ver much if the increment amount is already a constant.  But it can help e.g. change an addrec from `{0,+,%arg}` (`%arg` being a value whose def is outside the loop) to `{0,+,1}` which is useful.


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


================
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:
> > 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?


Repository:
  rL LLVM

https://reviews.llvm.org/D38494