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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6670
 
-  auto *BLoop = LI.getLoopFor(B->getParent());
-  if (BLoop && BLoop->getHeader() == B->getParent() &&
-      BLoop->getLoopPreheader() == A->getParent() &&
-      ::isGuaranteedToTransferExecutionToSuccessor(A->getIterator(),
-                                                   A->getParent()->end()) &&
-      ::isGuaranteedToTransferExecutionToSuccessor(B->getParent()->begin(),
-                                                   B->getIterator()))
-    return true;
-  return false;
+  // First find a path from B to A where if all blocks along path
+  // are transparent, then we can prove A reaches B.  Defer the actual
----------------
mkazantsev wrote:
> Quick check: `A` dominates `B`?
Two answers:
As actually used, that's trivially true.

For the interface, we don't need this property.  Consider B in the header of some loop L, with A in the (unconditional) latch block.  A must reach B even though A does not dominate B.  

This could be an optimization since the case implemented doesn't catch the case just mentioned.  Will add with that as a comment.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6689
+
+  while (true) {
+    Path.push_back(PrevBB);
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > I guess this will hand if you call it on 2 unreachable blocks going to one another.
> hang*
Good catch, we should filter by reachability since unreachable CFGs are weird. 


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6716
+  // local search, we use the block local cache as a filter.
+  auto doLocalSearch = [&](BasicBlock::const_iterator Begin,
+                           BasicBlock::const_iterator End) {
----------------
mkazantsev wrote:
> nit: capitalize lambda name.
They're methods, not variables.  (Or at least, I believe that's our style convention.)


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6847
       for (auto &I : *BB) {
-        if (!isGuaranteedToTransferExecutionToSuccessor(&I))
-          LP.HasNoAbnormalExits = false;
+        if (!llvm::isGuaranteedToTransferExecutionToSuccessor(&I))
+          Local.HasNoAbnormalExits = false;
----------------
mkazantsev wrote:
> Does it handle `invoke` terminator correctly?
Er, it's calling the same function as previously?  Not sure what you mean?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7660
+    for (auto *BB : CurrL->getBlocks())
+      BlockTransferExecutionToSuccessorCache.erase(BB);
     LoopPropertiesCache.erase(CurrL);
----------------
mkazantsev wrote:
> What about preheader?
Good catch, had not considered the implication of this combined with the attempted caching of the last element in the path.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111353