[PATCH] D62939: Fix a bug w/inbounds invalidation in LFTR (was: Strengthen LFTR's ability to replace IVs)

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

I was going to suggest that we only perform the poison/UB check if the GEP actually is inbounds... but I guess that wouldn't be quite right, as the poison can also come from the start value rather than the increment.

Let me try to summarize all the problems we can run into and how they will be handled as of this patch. Thankfully, our IV always has the simple form `IV = {S,+,1}`. Then, there are the following cases:

1. `S` may be undef. Handled by hasConcreteDef().
2. `S` may be poison. For pointers: Poison/UB check or prevent LFTR. For integers: **Not handled**.
3. `IV` may become poison due to nowrap/inbounds on last iteration. For pointers: Poison/UB check or use pre-inc. For integers: Recompute nowrap flags.
4. `IV` may become poison due to nowrap/inbounds on arbitrary iteration. For pointers: Poison/UB check or prevent LFTR. For integers: Recompute nowrap flags, but recomputation is not fully correct right now.

Notably, we don't handle case 2 for integers. We could drop the pointer type check. Probably better would be to instead handle cases 1&2 together by turning hasConcreteDef() into isNeverUndefOrPoison(). This would be similar to hasConcreteDef(), but also returning false for instructions with poison flags (I think that hasConcreteDef() should be doing that even when checking for undef, because ultimately we'll end up relaxing poison to undef anyway, due to lack of poison representation in IR.) An important case where isNeverUndefOrPoison() returns true is a constant start value, which many of the test cases have, I believe.

Then, we could split handling as follows. First, determine whether an IV can be used at all:

- If pre-inc IV (or a poison-propagating dependent value, including the post-inc IV) is used in current loop condition: Can use IV.
- Otherwise, if S is may be undef or poison: Don't use IV.
- Otherwise, if integer IV or pointer IV without inbounds: Can use IV.
- Otherwise, if UB check on pre-inc IV succeeds: Can use IV.
- Otherwise, don't use IV.

Second, determine what to do about post-inc IV. For integers are recompute flags. For pointers:

- If post-inc IV (or a poison-propagating dependent value) is used in current loop condition: Can use post-inc.
- Otherwise, if UB check on post-inc IV succeeds: Can use post-inc.
- Otherwise, use pre-inc.

I think that this should be close to the best we can do, though things are getting quite complicated here.

(Not saying we should do this in this revision, I'm just trying to sort out my thoughts.)


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

https://reviews.llvm.org/D62939