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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Inline comments on the SCC implementation.  As stated in previous comment, I think we should take an alternate approach, but leaving the comments since I took the time to build the mental state on the code.



================
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.
----------------
This case results in the new code being strictly inferior to the old code on large phi nodes.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6304
+      if (auto *PhiOp = dyn_cast<PHINode>(&*Op)) {
+        if (PendingPhiRanges.count(PhiOp))
+          continue;
----------------
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, 


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6314
+  SCC.insert(Phi);
+  Worklist.push_back(Phi);
+  // Out of reachable nodes, find those from which Phi is also reachable.
----------------
Instead of reusing the worklist variable, please scope the code such that you can use two worklists.  Makes tracking the flow much easier.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6372
+    else {
+      auto SharpenedRange =
+          I->second.intersectWith(ConservativeResult, RangeType);
----------------
This bothers me for two reasons: 1) It implies we sometimes don't recognize a phi node as being part of an SCC, and sometimes do, and 2) this is spooky action at a distance where later queries change cached results.

I strongly suspect this is masking a bug above.  


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

https://reviews.llvm.org/D110620