[PATCH] D63686: [LFTR] Fix post-inc pointer IV with truncated exit count (PR41998)

Sat Jun 29 02:49:53 PDT 2019

nikic added a comment.

In D63686#1562925 <https://reviews.llvm.org/D63686#1562925>, @reames wrote:

> I'm hesitant about this.  Not because your patch is wrong, but because we've ended up with a ton of complexity involving the post-increment case.  I'd really like to take a step back and see if we can factor apart some code here.
>
> Assume for the moment that we have a transform which knows how to generate the pre-increment form, can we do post increment conversion as a post processing step?
>
> If we were willing to strip, I think we could just strip flags and add one to both sides of the comparison *in whatever bitwidth the pre-inc form chose*.  Do you agree?
>
> If so, then I think we can move the post-inc legality/stripping entirely into a separate transform which runs *after* we've formed the arguments for the new comparison.  (Which is slightly different than the current patch as we'd do the pre-to-post *after* the extend/trunc decision.)
>
> I'm also really questioning the value of doing pre-to-post conversion in the middle end at all.  I'm getting increasing tempted to move that to just before codegen, and just strip all the flags without concern.  :)

I've been thinking the same thing ... the post-inc conversion here causes a lot of issues for unclear benefit. Some thoughts:

- LFTR currently does two things: Simplify an existing exit condition, or switch to a different IV. If we're simplifying an existing post-inc exit condition, we likely want to preserve it in that form. Though I think that this may be a good bit simpler than the general transform as we shouldn't have to deal with different-width IV and exit count in that case (is that correct?) Generally splitting the exit condition simplification and the IV switch parts of LFTR may be worthwhile, because the latter is much more dicey due to all the undef/poison baggage it comes with.
- Ideally we would not use post-inc form in the middle end and let LSR move to post-inc if profitable. Apart from the issues we're seeing here, this post-inc form is also non-canonical -- clearly `(a + C1) == C2` should canonically be `a == C2-C1`, but we have hacks in InstCombine to preserve this for multi-use cases. There is some relevant discussion on D58633 <https://reviews.llvm.org/D58633> and in particular some samples where using pre-inc form currently causes regressions. The first step would probably be to figure out what is going on there.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63686