[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:4085
+/// condition of loop latch. If there is a match we assume a true value
+/// for the condition while building SCEV nodes.
+class SCEVBackedgeConditionFolder
----------------
sanjoy wrote:
> I'd emphasize that this replacement is valid only in some specific cases (the backedge increment of an add recurrence).
Returning a SCEV for a true or false value by checking backedge condition of the loop latch is generic theme, which is what comment states.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4089
+public:
+  SCEVBackedgeConditionFolder(const Loop *L, ScalarEvolution &SE)
+      : SCEVRewriteVisitor(SE), L(L) {
----------------
sanjoy wrote:
> This can be `private`.  Please also make it `explicit`.
Yes, infact other visitor SCEVShiftRewriter is having same problem. Will be fixed as NFCI.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4095
+        BackedgeCond = BI->getCondition();
+        IsPositiveBackedgeCond = BI->getSuccessor(0) == L->getHeader();
+      }
----------------
sanjoy wrote:
> I think you also need to check that the false edge actually leaves the loop.  If both the edges out of the conditional branch branch to the header then this rewrite is not valid.
> 
> (Please also add a test case for the above situation ^).
Can you give a scenario where this can happen ? A latch where both the branches have same desitination header ?  


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4102
+                             ScalarEvolution &SE) {
+    SCEVBackedgeConditionFolder Rewriter(L, SE);
+    return Rewriter.visit(S);
----------------
sanjoy wrote:
> You can bail out early if `BackedgeCond` is `nullptr`.
Done


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4118
+          Value *TrueVal = I->getOperand(1);
+          Value *FalseVal = I->getOperand(2);
+          Result = SE.getSCEV(IsOne ? TrueVal : FalseVal);
----------------
sanjoy wrote:
> Use `getTrueValue()` and `getFalseValue()` instead of hard-coding the operand indices.
Done.


https://reviews.llvm.org/D38494