[PATCH] D42417: Re-apply [SCEV] Fix isLoopEntryGuardedByCond usage

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

Sorry, I incorrectly assumed that this was fixing a failed assert so I was looking for a crash.  I am able to reproduce the crash after reverting this change but adding back the isAvailableAtLoopEntry assert only.  Moreover, I think the assert is sound.

Having said that, I think this is a generic bug with how `isKnownPredicate` handles predicates between add recurrences.  What it used to do before your change doesn't make sense to me for three reasons:

1. It tries to do this weird double induction on the two add recurrences.  However, it does not check that the two add recurrences are from the same loop, and without this precondition the function is definitely buggy.
2. Even if we fix (1) we are still checking a stronger condition that necessary (and in some cases it may even be too weak a.k.a. incorrect, but I've not thought deeply about this).  I think we should be invoking induction on both the add recurrences at the same time.
3. It only look at the outermost operation of LHS and RHS.  It should instead by checking if the LHS and RHS are *functions* of add recurrences.

So I think the right fix involves doing the following:

1. Add the isAvailableAtLoopEntry assert to isLoopEntryGuardedByCond.  I think that assert is sound.
2. In isKnownPredicate, find the deepest loops in LHS and RHS.  Call then L_L and L_R. The invariant is that one's header has to dominate the other, or L_R == L_L.
  1. If L_R == L_L then check if entry to L_L is guarded by First(LHS, L_L) `pred` First(RHS, L_R) and backedge is guarded by PostInc(LHS, L_L) `pred` PostInc(RHS, L_R).
  2. If L_L dominates L_R then check if entry to L_R is guarded by L_L `pred` First(L_R) and if backedge is guarded by L_R `pred` PostInc(RHS, L_R) [i.e. treat RHS as an induction variable, and LHS as a loop invariant value].
  3. Symmetric clause as (B) if L_R dominates L_L.

Where First(X, L) is defined as "in X replace all add recs with loop L with their initial value" and PostInc(X, L) is defined as "in X replace all add recs with loop L with their post-inc value".  We may already have SCEV rewriters that do this.

What do you think?


Repository:
  rL LLVM

https://reviews.llvm.org/D42417