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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6266
+              if (PendingPhiRanges.count(PhiOp))
+                continue;
+              if (Reachable.insert(PhiOp).second)
----------------
reames wrote:
> 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.  
Basically, the algorithm is correct even if we ignore random nodes. The phis that get into `SCC` all:
- Are reachable from first `Phi`;
- Use first `Phi` (directly or indirectly).
It means that in any case SCC is some strongly connected component, even if not maximal by inclusion. It's OK. Our statement that `SCC` Phis can only take values that come to them from outside is still true.

In practice, if we ever meet a pending input, it will have range of full set and entire thing will also be full set.


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

https://reviews.llvm.org/D110620