[PATCH] D111353: [SCEV] Extend ability to infer flags to more complicates scopes

[Philip Reames via Phabricator via llvm-commits]

reames planned changes to this revision.
reames added a comment.

Placing this on hold.  I was expecting to see perf regressions reported against the flag redefinition changes, and this was expected to be needed to help claw back some of that performance.  As there's been nothing reported, I'd prefer not to add further complexity here without cause.  Placing this on hold for the moment, if we go a couple more weeks without reported regressions I'll abandon this change.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6660
+  // require reachability
+  if (!DT.isReachableFromEntry(B->getParent()))
+    return false;
----------------
mkazantsev wrote:
> Makes sense to check reachibility of `A` as well, just to avoid useless work and save some CT.
We know A dominates B.  From B being reachable, A must also be.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6675
+    auto *PrevBB = BB->getUniquePredecessor();
+    return PrevBB && PrevBB->getUniqueSuccessor() ? PrevBB : nullptr;
+  };
----------------
mkazantsev wrote:
> As an idea for follow-up: if `PrevBB` is the only exit of a loop (w/o abnormal exits or locks etc), we can also return it, because the loop should be finite. Maybe makes sense to add a TODO.
We have this same basic pattern repeating in a bunch of places.  If we ever implement this idea, we'd need to visit all of them.  I don't think having a comment in one place really helps.  


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

https://reviews.llvm.org/D111353