[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 a comment.
This revision now requires changes to proceed.

Mostly minor stuff remains + the one correctness issue with a conditional latch branching to the header in both the true and false cases.



================
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:
> 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.
The cast `isa` seems redundant here -- can't you return a value of type `SCEVConstant` (that may or may not be null)?

Though I'd still prefer `Optional<bool>` since that's more specific -- reading `SCEVConstant` you can't immediately tell that `CondSE` is either `1` or `0`; but with `Optional<bool>` that fact will be obvious from the type system.


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


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4124
+        if (ICmpSE && isa<SCEVConstant>(ICmpSE))
+          Result = ICmpSE;
+        break;
----------------
The `ICmp` prefix to `ICmpSE` is misleading, since the condition does not need to be an `icmp`.


https://reviews.llvm.org/D38494