[PATCH] D110620: [SCEV] Infer ranges for SCC consisting of cycled Phis

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Seems overall reasonable, though, I'm curious, where do you see this trigger in practice?  Given this formulation doesn't involve anything but phis, it disallows IVs.  Given that, I'm curious what patterns create this.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6245
     if (const PHINode *Phi = dyn_cast<PHINode>(U->getValue())) {
-      // Make sure that we do not run over cycled Phis.
-      if (PendingPhiRanges.insert(Phi).second) {
-        ConstantRange RangeFromOps(BitWidth, /*isFullSet=*/false);
-        for (auto &Op : Phi->operands()) {
-          auto OpRange = getRangeRef(getSCEV(Op), SignHint);
-          RangeFromOps = RangeFromOps.unionWith(OpRange);
+      if (!PendingPhiRanges.count(Phi)) {
+        // Collect strongly connected component (further on - SCC ) composed of
----------------
This block of code has gotten quite complicated, probably time for a helper function.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6266
+              if (PendingPhiRanges.count(PhiOp))
+                continue;
+              if (Reachable.insert(PhiOp).second)
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > nikic wrote:
> > > Why is it okay to ignore operands in PendingPhiRanges here?
> > They are not ignored, they are some SS Phi's input so they will be in `Roots`. We will take the range from them for our union.
> SCC Phi*
I'm not sure about this statement.  IF this is the *first* recursion (e.g. the top level value is a phi) in this SCC then sure, but we'd analyze something which happened to contain a non-SCC phi, then recursed into it's operands (which contained an SCC phi), I'm unsure.  At a minimum, I think the code would be easier to follow if you seperated the SCC detection from the PendingPHIRange mechanism.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6327
+          else {
+            auto SharpenedRange =
+                I->second.intersectWith(ConservativeResult, RangeType);
----------------
Does this ever get hit?  I'd expect this to be unreachable as we'd always traverse the same SCC by definition from any entry.  


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110620/new/

https://reviews.llvm.org/D110620