[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:4099
+        IsPosBECond = BI->getSuccessor(0) == L->getHeader();
+      } else
+        return S;
----------------
sanjoy wrote:
> It is usually customary to to put the `else` block in braces if the `if` is in braces.
Actually I purpusfully wrote it this way based on comments received on other patches reviewed. Shall change here following the file convention.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4108
+    const SCEV *Result = Expr;
+    bool InvariantF = SE.isLoopInvariant(Expr, L);
+
----------------
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.


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




https://reviews.llvm.org/D38494