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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4115
+      case Instruction::Select: {
+        const SCEV *CondSE = visit(SE.getSCEV(I->getOperand(0)));
+        if (isa<SCEVConstant>(CondSE)) {
----------------
sanjoy wrote:
> Calling `getSCEV` + `visit` seems a bit overkill -- can't you just compare `I->getOperand(0)` with `LoopBackOnPosCond` here?
> 
> How about adding an `Optional<bool> isValueKnown(Value *Cond)` function that returns `true` or `false` if `Cond` is equal to `LoopBackOnPosCond` else returns `None`, and using that function in both the cases?
This comment needs to be addressed -- I don't think we should be creating `SCEVUnknown` unnecessarily as you're doing here.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:6518
 }
 
+
----------------
sanjoy wrote:
> Whitespace damange?
Wasn't fixed.


================
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
----------------
I'd emphasize that this replacement is valid only in some specific cases (the backedge increment of an add recurrence).


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4089
+public:
+  SCEVBackedgeConditionFolder(const Loop *L, ScalarEvolution &SE)
+      : SCEVRewriteVisitor(SE), L(L) {
----------------
This can be `private`.  Please also make it `explicit`.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4095
+        BackedgeCond = BI->getCondition();
+        IsPositiveBackedgeCond = BI->getSuccessor(0) == L->getHeader();
+      }
----------------
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 ^).


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


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


https://reviews.llvm.org/D38494