[PATCH] D100004: [SCEV] Correct handling of recurrences matched in partially unreachable code (try 2)

[Philip Reames via Phabricator via llvm-commits]

reames abandoned this revision.
reames added a comment.

I prototyped a couple of alternatives to this, and the resulting interface in matchSimpleRecurrence is ugly and error prone.  Based on a survey of the existing users, SCEV is the only place which cares about the mismatch between LoopInfo and reachability.

I'm strongly of the opinion that the loop info approach sketched here is the right one, but given this seems to have stumbled into a much larger issue (with, IMO, incorrect docs and missing asserts) I'm going to set this aside.  I already have several major changes in flight and don't have the bandwidth for any more.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5675
   auto *L = LI.getLoopFor(P->getParent());
-  assert(L && L->getHeader() == P->getParent());
-  if (!L->contains(BO->getParent()))
+  if (!L || L->getHeader() != P->getParent())
+    // If we have unreachable blocks involved, then dominance collapses and
----------------
mkazantsev wrote:
> reames wrote:
> > mkazantsev wrote:
> > > reames wrote:
> > > > mkazantsev wrote:
> > > > > Do we have a guarantee of having no loops in unreachable code?
> > > > See LoopInfoBase<>::analyze specififically the line:
> > > > DomTree.isReachableFromEntry(Backedge))
> > > > 
> > > > Note that the verify routine also checks against a newly constructed LI, so any failure to update should be caught in a expensive asserts run.
> > > But expensive verification runs between the passes. It is still possible that some pass makes a loop unreachable in the middle of the transform and does not immediately destroy the loop while continuing to use SCEV.
> > > 
> > > I am still strongly leaning towards not relying on such things, even if they might be correct. It's far from obvious and far from simple.
> > Passes that rely on running with partially corrupt analyzes are buggy.  We should find and fix them.
> It is not a corrupt analysis. It is clearly stated that LI for unreachable blocks is undefined. It may have loops anywhere.
IMO, this is a case where the docs are simply wrong.

The (lack of an) invariant your arguing for here would make it hard to implement any useful analysis based on loops.  If we redefined dominance in unreachable code, maybe we could make this work, but with our current domtree semantics not having loopinfo match that is a serious problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100004