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

Max Kazantsev via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 2 23:14:33 PST 2021


mkazantsev added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6296
+  while (!Worklist.empty()) {
+    if (Reachable.size() > MaxPhiSCCAnalysisSize) {
+      // Too many nodes to process. Assume that SCC is composed of Phi alone.
----------------
reames wrote:
> This case results in the new code being strictly inferior to the old code on large phi nodes.  
Two points here:

1. Old code, seeing cycled phis, will always return full set, which in union will also give fullset for all phis. So no, in this case it's not worse, because "worse" is impossible.

2. Old code, not seeing cycled phis, will do exactly same thing as this one (e.g. process phi itself as the sole root).

So no, if I'm reading this correctly, it is strictly not worse than the old one.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6304
+      if (auto *PhiOp = dyn_cast<PHINode>(&*Op)) {
+        if (PendingPhiRanges.count(PhiOp))
+          continue;
----------------
reames wrote:
> I'm not sure simply ignoring an input is correct here.  We may have to abort instead.
> 
> Hm, I think you're intent is that these get treated like external roots to the SCC.   This a) probably explains why you get inconsistent SCCs over time, and b) deserves a comment at minimum, 
Off course it is correct to cut in any arbitrary place. We can take any subset of Phis, even if it's not a SCC, collect all inputs from outside of this subset, and use their union as estimate of the range of all these Phis. But it's only profitable when this subset is a SCC.


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

https://reviews.llvm.org/D110620



More information about the llvm-commits mailing list