[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:4083
 
+class SCEVCmpEvaluator : public SCEVRewriteVisitor<SCEVCmpEvaluator> {
+public:
----------------
I think a better name would be `BackedgeConditionFolder`.  Please also add a one-liner describing what it does.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4100
+    if (!InvariantF && Expr->getValue() && isa<Instruction>(Expr->getValue())) {
+      Instruction *I = dyn_cast<Instruction>(Expr->getValue());
+      switch (I->getOpcode()) {
----------------
You can just use a `cast<>` since you've already checked `isa<Instruction>`.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4103
+      case Instruction::Select: {
+        const SCEV *ICmpSE =
+            SE.evaluateCompare(cast<ICmpInst>(I->getOperand(0)));
----------------
Not all select condition operands are icmp instructions.

I also don't think you need the condition to be an icmp instruction -- if the condition is the latch condition, then on increment expression it is going to be true.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4817
         if (i != FoundIndex)
           Ops.push_back(Add->getOperand(i));
+      Accum = getAddExpr(Ops);
----------------
I think it would be simpler if you changed `SCEVCmpEvaluator::rewrite` to not return CouldNotCompute and always just return the rewritten value; and here did:

```
Ops.push_back(SCEVCmpEvaluator::rewrite(Add->getOperand(i)));
```



================
Comment at: lib/Analysis/ScalarEvolution.cpp:6512
+
+  // If compare instruction is same or inverse of the compare in the
+  // branch of the loop latch, then return a constant evolution
----------------
I'd suggest putting this method in `SCEVCmpEvaluator`; having it on `ScalarEvolution` will be prone to misuse.  Once you do that, you can probably make the convention a bit simpler and have it return a `SCEVConstant *` which may be null to indicate failure to fold.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:6514
+  // branch of the loop latch, then return a constant evolution
+  // node. This shall facilitate computations of loop exit counts
+  // in cases where compare appears in the evolution chain of induction
----------------
This comment needs to be more specific on why the transform is correct -- it isn't that the latch condition is always true, but we know it is true **if** the backedge is taken.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:2974
+        const SCEV *SI = SE.getSCEV(&I);
+        if (!isa<SCEVUnknown>(SI) && !isa<SCEVConstant>(SI))
+          continue;
----------------
Why do you need this change?


https://reviews.llvm.org/D38494