[PATCH] D69009: [IndVars] Eliminate loop exits with equivalent exit counts

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LG



================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2796
+      continue;
+    }
   }
----------------
nikic wrote:
> reames wrote:
> > nikic wrote:
> > > Now that we're traversing the exits in dominating order, I think we should consider a slightly different overall approach:
> > > 
> > > Instead of computing `umin({ exit counts })` upfront and then checking for `umin({ exit counts }) < this exit count`, we can instead build this up incrementally, i.e. take the umin between the current max exit count and the exit count of the currently considered exit at each iteration. This will allow us to check for `umin({ already seen exit counts }) <= this exit count` (note the `<=`!) instead.
> > > 
> > > This should implicitly handle the case of exact equalities, but may also be more powerful in general because we're checking for a weaker predicate.
> > The suggested approach doesn't handle one of the most common cases.  If the latch exit is in fact the minimum exit, we want to be able to discharge earlier exits.  (i.e. the difference between the dominating set and the total sets is important)
> > 
> > I do agree that your phrasing *might* be more powerful for eliminating exits which SCEV can prove are <= dominating checks (but not either < or ==).  However, I don't have a test case which demonstrates that difference, and until I do, I'd like to avoid further complicating the code.  (Assuming I'd need both approaches due to the gap described above.)
> For an example, consider the existing `@ult` test just using a `ule` predicate instead: https://godbolt.org/z/j-rZeb This should eliminate the second exit, but doesn't, as we have neither the `<` predicate nor the strict scev identity (but do have a `<=` predicate).
> 
> I didn't quite follow what you mean regarding the latch exit and why it is special. Is there a test case that shows the issue?
Okay, I see what you mean now. Not specifically the latch, just any future exit. It does seem like we'd have to combine both approaches to get the full coverage, so let's go with your current variant for now.


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

https://reviews.llvm.org/D69009