[PATCH] D110620: [SCEV] Infer ranges for SCC consisting of cycled Phis
Philip Reames via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 30 09:43:29 PST 2021
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
More information about the llvm-commits
mailing list