In D63401#1547122, @arsenm wrote:

In D63401#1547107, @reames wrote:

In D63401#1546293, @sanjoy wrote:

I don't have opinions on this, but Philip might have comments around whether this is okay for GC pointers.

No opinion GC wise, but mostly because I don't understand the intent of this patch. What is the desired outcome here?

For an equivalent bitcast, the type of the underlying alloca is changed into a nicer type and possibly split into multiple allocas (with volatile accesses). I conservatively added checks in r363462 to avoid using the alloca's natural address space for the new operations. I figure it's safer to not change the address space of volatile accesses, but I don't particularly care about this property. InferAddressSpaces avoids changing volatiles, but that was also my idea.

So, just to say this back to you, this would be treating an addrspacecast more like a bitcast when the source is known to be an alloca?

Again, LGTM.

On hold given previous review comments.

No longer actively pursuing. The idea is reasonable, and only partly overlaps with LFTR, but all of the non-reduced motivating cases are covered by multiple exit LFTR.

In D63401#1546293, @sanjoy wrote:

I don't have opinions on this, but Philip might have comments around whether this is okay for GC pointers.

No opinion GC wise, but mostly because I don't understand the intent of this patch. What is the desired outcome here?

LGTM

Wow, we have a huge amount of boilerplate when adding an attribute.

In D61652#1540764, @jdoerfert wrote:

I'm working on all the remaining changes now and I'll update this patch with the mechanics first before I create a new one to move the users.
We can wait with committing both until we merged the nofree and nosync patches and I updated the isDerefPointer helper function to use them.
Would that be OK?

Absolutely. That's exactly what I'm asking for.

rebase on requested tests

Sorry for the late reply. Replying selectively to key points.

LGTM w/a caveat. I fully expect that landing this would expose lots of existing breakage and would break the build bots. In practice, I don't think you should land this until fixing any issues you've already found. Once that's done, land, but expect to revert and fix found issues for a few cycles.

LGTM w/two required changes.

LGTM w/two required changes. (If you want to just make the changes, you can submit without further review. )

Incorporate Nikita's suggestion about IVs which are already post-increment, and just need their formed changed.

In D62939#1540490, @nikic wrote:

If the mustExecuteUBIfPoisonOnPathTo() check is performed, then, as you argue, we do handle S being poison properly. It's just that the check is only performed on pointers, but not integers (in this patch).

Ah, agreed.

A suggestion on how to make progress here. Introduce the wrap flags to the API, but have *all* callers pass AnyWrap. That must be NFC.

Rebase on landed prep patches. Ready for actual review, though won't land until after https://reviews.llvm.org/D62939.

In D62939#1538744, @nikic wrote:

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:
....

2. S may be poison. For pointers: Poison/UB check or prevent LFTR. For integers: Not handled. ...

Actually, think 'S' being poisoned is almost fully handled or maybe even fully handled. If 'S' is poison, then IV must be poison on at least the first iteration. Given that, if we find a side effect which must execute on the path to our exit block, then we've proven we must have executed UB. As such, the program is undefined and we have no further obligations.

Planning a change in direction after offline discussion w/Artur. Instead of framing this as hoisting/widening, I'm going to reframe as a separate loop transform which lifts a trivial guard (by splitting the condition) into the loop preheader where possible. We think this will be easier to test, and may have positive effects beyond guard widening,

Rebase over tweaked tests and address comments from Nikita.

First iteration on a combined patch. The tests need updated to reflect the poison/undef split. I'll do that, then rebase on top.

The issues raised in this and D62939 turned out to be inseparable. Given the meaningful review discussion happened on the other review, I'm abandoning this patch.

Minor rebase. As pointed out by previous comments both from myself and Nikita, the entire approach here is broken. This doesn't actually appear to be separable from D62936. Given all the meaningful review has happened here, I'm going to repurpose this review for the bug fix and close that one.

LGTM for the AtomicExpand parts. I'm willing to assume you know what you're doing for the AMDGPU bit. :)

Rebase on top of newly added tests.

Also LGTM. Nice cleanup.

Remove the buggy phi code Nikita pointed out and otherwise address last round of comments from Nikita.

Remove the two cases Nikita noted.

Blocked on discussion in D62939 and landing thereof.

Flags on SCEVs are valid for the current set of users that exist in the loop. If you add *new* users then those flags may no longer be valid. We recently ran into this in LFTR, and this patch seems likely to have the same risk. I have not audited the patch or its output to be sure, just throwing out a possibility.

Revise comments per Nikita's suggestions and some further word smithing of my own.

Address a few of Nikita's comment. I ended up heavily revising the structure, so the line locations got a bit confusing.

Cleanup code and fix a couple of bugs in the process.

The NFC portion of this turned out to be a bit more involved than expected. It's been separated into D62880. Once that lands, I'll rebase for the small remaining functional bit. I'm debating just going to all loop exits instead of just the latch exit. On further reflection, I'm not sure it's really going to matter impact wise, and fleshing out problems faster might be worthwhile.