[PATCH] D44001: [SCEV] Prove implications for SCEVUnknown Phis

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:9583
+    if (auto *Phi = dyn_cast<PHINode>(LU->getValue())) {
+      if (!PendingMerges.insert(Phi).second)
+        return false;
----------------
Could you pls give an idea of how these `PendingMerges` are used?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:9615
+
+  if (RPhi && LPhi->getParent() == RPhi->getParent()) {
+    // Case one: RHS is also a SCEVUnknown Phi from the same basic block.
----------------
Nit: `LBB == RPhi->getParent()`


================
Comment at: lib/Analysis/ScalarEvolution.cpp:9618
+    // If we compare two Phis from the same block, and for each entry block
+    // the predicate is true for icoming values from this block, then the
+    // predicate is also true for the Phis.
----------------
incoming


================
Comment at: lib/Analysis/ScalarEvolution.cpp:9633
+    auto *Preheader = RLoop->getLoopPreheader();
+    assert(Preheader && "Loop with AddRec with no preheader?");
+    const SCEV *L1 = getSCEV(LPhi->getIncomingValueForBlock(Preheader));
----------------
Can we also add an assert that the `num of Incoming Values` for LPhi is exactly 2?

Really just seems like RAR is a special case of a SCEV Phi with 2 incoming values. We just don't have a Phi node, it's recorded as an RAR. 


================
Comment at: lib/Analysis/ScalarEvolution.cpp:9643
+  } else {
+    // In all other cases go over inputs of LHS and compare each of them to RHS,
+    // the predicate is true for (LHS, RHS) if it is true for all such pairs.
----------------
Could you state here: `LHS is phi and RHS is non-phi`. It's obvious when looking at above code and the swapping, but easier when seeing this as a standalone third case.


https://reviews.llvm.org/D44001