LGTM too.

Thanks for this! LGTM after erichkeane's comments are resolved.

Shrunk the diff as requested; the few pre-commit patches required to make this build are available as one patch in https://reviews.llvm.org/D52245 .

This looks good to me, but like said, I don't have sufficient context to stamp it with great confidence.

LGTM, modulo one nit. Thanks!

Thanks for this!

LGTM. Thanks for this!

Thanks!

I have zero context on the various ways we can/do add instructions to a BB, so I can't immediately be helpful with the tricky part of this review. :) I've no complaints about the general direction of this patch, though.

LGTM after the requested nit. Thanks again!

LGTM. Thanks again!

Thanks for this!

Thanks for this!

Lots of nits, and a few functional concerns. For the functional concerns that revolve around dubious edge additions, I'm happy with fixes or "it's impossible for that to actually happen; added a comment explaining that."

Looks fine to me, though I'd prefer if someone familiar with this code had a look, as well.

LGTM with a few comments. Thanks!

I'm not entirely through this patch (I hope to make more progress in the coming few days), but this is what I have for now. Logic's looking solid so far; I'm just nitpicky. :)

George, let me know if you agree with using the generic one and I'll abandon this revision. Thanks!

Never touched this code, so all I can offer is a style nit. Thanks for the patch!

Workaround committed as r339411

(FWIW, I plan to commit a workaround like this tomorrow with said big scary comment, assuming no complaints appear before then. Like said, I'm more than happy to discuss the ideal path to take here even after that lands. But this is a known miscompile with a trivial workaround that should presumably also find its way into the release branch, so...)

Currently, MSSA treats lifetime intrinsics as clobbers, even though they're technically not. This patch would further solidify that behavior, which might not be a good thing (e.g. there's a good argument that we shouldn't be treating lifetime intrinsics as clobbers in the first place). I'm happy to rip out what few lifetime-related pieces MSSA has, but before doing so:

ping :)

Thanks for this! In addition to Hal's comments...

Thanks!

Ping :)

Thanks for this!

Thanks for this!

Looks straightforward enough to me, but spatel authored this pass (and I've never touched it), so I'm going to defer to them :)

Thanks again!

Ping :)

As a result, I also reverted the changes to make CounterDesc part of CounterInfo.

Running ninja check-llvm after applying this on top of r337631, I see many test failures in CodeGen/AMDGPU. Are you seeing these, as well?

This LGTM; thanks again for the patch. Will land it for you once davide's happy

LGTM after one nit is addressed. Thanks again!

Thanks for this!

As all the PhiOps are collected we call tryRemoveTrivialPhi which might have replaced the Phi with another MemoryAccess. Then we try to cache that MemoryAccess with the same BB key (line 110), which should return false

Thanks for this!

I don't really like adding arbitrary flags to existing passes; better to have a separate pass, I think.

Switch to having a pass to do this, as requested.

LGTM. Thanks again!

Thanks again!

It also adds and additional overhead

Ping :)

LGTM with a few comments; thanks again!

I'm still in favor of just replacing SmallVector<MemoryPhi *, 8> InsertedPHIs with SmallVector<WeakVH, 8> InsertedPHIs, since:

• I think it meets all of our needs here,
• it's a very straightforward change (no need to deal with removal, new containers, etc.), and
• I don't see how we can ever get a duplicate in InsertedPHIs to start with.

(I'll go ahead and commit assuming my interpretation of arsenm's message was correct. Happy to revert or land follow-ups if not)

Thanks for the patch!

Nit fixes LGTM. Please wait for an official stamp from Eli before committing, since I can’t tell if he got the second look he wanted. (I’m inclined to believe he did, but don’t want to jump the gun)

Ah, sorry. I thought I hit submit, but apparently I didn't. :)

What Eli said :)

Swooping in on this admittedly quite late: should the objectsize bits in MemoryBuiltins.cpp be made aware of this?

Thanks!

Ping :)

Rebase + address feedback

LGTM with one remaining nit. Thanks again!

Thinking out loud: I wonder how often people ask for insertion order but only actually care about having a consistent order.

Thanks for this!

Thanks for this!

LGTM after two nits are addressed. Thank you!

LGTM after a nit is fixed; thanks!

This revision was not accepted when it landed; it landed in state Needs Review.

(Committed as noted by echristo; just trying to clean my queue a bit. :) )

(Committed as noted by echristo; just trying to clean my queue a bit. :) )

Ping :)

(Sorry for the double-post; one more bit that I noticed scrolling up)

This LGTM after one last ultra-small nitpick is addressed. I don't see you in the commit logs, so will you need me to commit this for you?

Hi, and thanks for this patch!

Rebase now that our LocationSize changes are in

Thanks for the clarification, and sorry about the misunderstanding. I submitted the bit-for-bit NFC patch as r333314, and will rebase the original review soon. :)

(FWIW, Alina and I chatted about this offline. She has a few changes queued up; I'm adding my nits on unchanged bits now, and will do a thorough code review once the new patch is posted)

WFM; thanks for checking!

Thanks!

(Test committed as r332017)

LGTM; thanks again.

Adding George to see if he remembers any cases, so you have some stuff to test this all with.

I don't know this code well enough to LGTM this, but can you talk a bit about why, please? This side-effect-inside-an-assert looks sane to me.

Hi, and thanks for the patch! I'm pretty familiar with this code and am happy to review it when I get back from vacation next week.

Accidentally left in LocationSize::precise. Oops. :)

I am concerned that you doing too many things in one patch and that the mix of refactoring and functional changes may contain hard to spot bugs.

(Thanks for the patch, by the way!)