This looks like a generally good improvement, but is it really the case that if any variable operand is a def that all must be? It would seem more general to have a list of variable operands defs and a list of variable operand uses. I have an alternate use case which would require that at some point, but I'm wondering about the current ARM backend use case at the current time.

LGTM w/minor comment to be addressed before landing.

Looks like a good step in the right direction after some cleanup. Any chance you're interested in doing the same for argument or return attributes?

This patch made me curious because I remembered fixing this one. Turned out that's still a patch we're carrying downstream. Oops. :(

LGTM in the current form. The alternate framing might also be good, but we can come back and undo this later if that works out.

In addition to the detailed implementation comment below, I thought of a possible semantic problem. Per the ABI, who "owns" the memory for the arguments? Does either the callee or caller assume it's immutable? If so, then your optimization needs to be restricted to a non-relocating collector since otherwise the collector might be updating a memory location assumed by AA to be immutable and bad things could happen...

LGTM. I'm actually a bit uncertain of the semantics of this change, but I doubt we're going to find a better reviewer, so I did my best. :)

We can continue the discussion around finalizeModule and emitStackMaps separately. It's definitely not a blocker for this patch.

A few drive by comments for the moment.

Drive by comments.

In D53786#1279569, @reames wrote:

This feels like possibly a solution in search of a problem?

One potentially problematic scenario in the current code would be if someone recursively deleted a user instruction inside an AST iteration loop. I think that would delete the pointer value, and if that was the last value in a given AS, invalidate the iteration.

Hm, can I ask what problem you were originally trying to solve? From a quick search, this code is only invoked through the value handle deleted callback. In which case, the value being deleted is the pointer itself, not the original using instruction.

p.s. This is an obvious NFC cleanup You'd be fine to land this without review.

Starting with only high level design comments....

LGTM

LGTM w/one optional, but strongly recommended change.

In D52760#1253459, @craig.topper wrote:

I definitely agree that this recursion is not the ideal way to implement this. I haven't looked closely to see if we always do this recursion or if we sometimes just return without recursing. Any issue with this patch in the short term?

I'm not actively objecting, but I'm not sure I'd bother to pursue this intermediate state as opposed to directly addressing the root issue. If you really want to pursue this approach, I'm willing to review and signoff.

Instead of doing the simplification recursively, wouldn't it be better to just iterate externally? That is, what if we split run into an outer function which called an inner helper routine as long as that inner routine was making progress? Or is there a concern that SimplifyCFG does not stabilize to a fixed point?

Mostly minor comments. I considered giving a conditional LGTM, but decided it warranted one last round. I think this is very close to landing.

Just noting a couple of known gaps (to save reviewers time):

• No verifier support (will add before submission)
• Need to add references from each support instruction type in the docs (done to avoid rebase headache)

Another round of bugs found.

I'm approving this in the current form, despite a bit of hesitation doing so. I'd like to see the conversation around the restriction of widening based on target block to continue. I think there's a good change we'll want to tweak the semantics there, but I see that as a minor tweak, not a major redesign.

Looked at the history in more detail, and decided I'm comfortable with this fix. So, LGTM.

This looks correct to me, but this is so glaringly bad I'd like a second set of eyes to make sure I'm not missing something. Can someone else confirm?

Overall, looks pretty good. One rounds of comments, but I'm expecting this to converge quickly.

LGTM w/ changes made before submit.

reverse last rebase, the small addition was buggy (iterator invalidation)

(slightly modified patch to reduce overhead for multiple predecessor case too)

rebase for minor issue I commented on, also ping?

No longer needed, and extremely stale.

All component pieces have been split and landed individually.

In D50925#1211383, @anna wrote:

Added example:

(snipped example for length)

In D50925#1211015, @anna wrote:

There are 3 kinds of tests worth adding:

1. predicated invariant stores, i.e. the block containing the store itself is predicated and not guaranteed to execute (cannot be handled by LICM)

Covered by existing early exit test.

2. invariant store value is a phi containing invariant incoming values and the phi result depends on an invariant condition (can be handled by LICM. This patch handles?)

Unclear what you mean here.

3. invariant store value is a phi containing invariant incoming values and the phi result depends on a variant condition (cannot be handled by LICM safely)

Again, I don't know what you mean by this. If the value is a phi in the loop, it's by definition not invariant.

with the right patch this time

rebase and incorporate suggestions.

Adding requested tests, ready for review.

Address first couple comments, another update to come before re-review justified.