Looks close to ready. Please address review comments, and we'll probably be ready for an LGTM.

address review comment

LGTM. This makes a nice starting point - as you note, we can definitely extend this.

LGTM - if you're just adding tests, you don't need separate review as long as there's nothing fancy.

Max, I think this is probably handled by the existing optimizeLoopExits if the exit counts for the condition are computable. (i.e. you're probably looking at a gap in exit count computation)

However, I think this a workaround (and a useful optimization) not a true fix. What would happen if for some reason a COPY didn't get eliminated? (Imagine no duplicates, just a single pointer being relocated, with relocated value used down the exceptional path) I don't have a test case for this, but I'd advise digging into that possibility further. I'd need to study invoke lowering more closely to understand the risk.

When rebasing, I discovered the tests for this were from before the operand bundle format change. Given that, I ended up dropping most of the tests. I'll land a couple more in a follow up, but overall, it seems reasonable to have most testing in the x86 target. We don't need to exercise all cornercases in both places.

LGTM as well.

LGTM

Must fix:

• Your change does not include a LangRef update for the new metadata.
• The name chosen for the metadata is ambiguous. I'd also suggest phrasing it in terms of ranges, not page starts. Maybe: guaranteed_faulting_ranges, or implicit_check_faulting_ranges?
• As you note in the review, this is mixing two sets of changes in a way which makes it hard to reason about either in isolation. Please split. Either order is fine as you should be able to test either piece in isolation without the other.

LGTM

LGTM

LGTM. Good catch, and nicely structured cleanup in the process.

Approach generally looks reasonable. Please address nitpicks and then I plan to LGTM.

LGTM

LGTM

As a follow up, it would be nice to handle two additional optimizations:

1. Canonicalize each pointer to a single index in the statepoint operand list
2. Shrink the gc-live bundle to remove any pointers not needed after (1)

Serguei and I discussed this one fairly extensively offline. Long term, we're probably going to move to a model where we process all of the projections for a statepoint in a single pass when visiting the statepoint, but for the moment, we need to decrease the number of instcombine iterations. In particular, with recent efforts to clamp the number of iterations in debug builds, we are seeing R+A crashes in downstream tests, so this change is really a regression fix.

LGTM. I sat down today to write a cleaner patch, and in the process convinced myself this was in fact correct. I will post a separate cleanup once this lands as I think we can have the foldMemoryOperand impl structured much more cleanly for tied operands, but doing so involves a slightly deeper rewrite of some x86 specific code than is reasonable to ask for here.

In D81648#2150053, @dantrushin wrote:

In D81648#2146051, @reames wrote:

Second, there appears to be a semantic problem around the handling of base vs derived slots unless we *always* spill the base. We can't tie both uses to a single def. This may warrant some offline discussion.

Yes, we can't. But apparently, we do not need to.
Basically, that tied-ness information is only needed to glue two live ranges into one during SSA flattening. And it does not matter how many uses end live range at the same instruction.
Luckily, LLVM appears to handle it properly.

But if included tests (and checks) do not make you believe it works, we should throw all this stuff away and try some different approach.

I filed a bug with a detailed description of the problem and one suggestion as to how to approach. See https://bugs.llvm.org/show_bug.cgi?id=46917, let's take discussion there (or offline).

JFYI, I've posted (and now landed after review) the subset of this patch which is mostly NFC here: https://reviews.llvm.org/D84692

Address Register related review comments

I'm approving this patch not because it's perfect, but because it doesn't break anything in the existing code and iterating here is no longer productive. Once this lands, we can continue the discussion about base/derived handling with specific concrete examples.

I took some time today to apply this patch with the attention of addressing some of the unaddressed comments and landing it to unblock progress here. However, I hit two problems.

(Comments on code outside StatepointLowering. Minor +1 question)

Detailed comments on the new implementation. These are on the whole minor. Remember to add your new test file.

On first skim, looks much much better. I'm going to do a detailed pass through, but thank you for making the major design change requested.

And thank you for doing this.

I've spent several hours today wrapping my head around this patch. I think I've found a material simplification which should greatly simplify the code.

Required Change - Please introduce a runtime flag which controls how many values are handled via vregs. Default this value to zero. This will remove all existing test diffs; if it doesn't you have a bug. Then introduce a new test file, optional copied from an existing one, called statepoint-gc-regs.ll which enumerates sufficient coverage for the new feature.

Initial comments, continue to look for other suggestions.

(drive by comments only, please don't block on me)

(Just for reference, the total bit tripped me up at first, but the reasoning is that we're converting to a call, not proving the call no throw. We could still throw out.)

Denis, I'm having a really hard time wrapping my head around this code, even knowing what it is supposed to do. We need to clean this up a bit; it's not in a state where I can approve it.

Just a thought, feel free to consider or ignore.

In D81646#2092782, @dantrushin wrote:

In D81646#2091262, @reames wrote:

I can't wrap my head around why untieing operands would be correct here. It seems like we could end up with a use folded, but a def not leaving to invalid codegen? Can you either expand on the justification or discuss offline?

The key point here is that we have two-address form at this point (not SSA, i.e. register on both sides is the same: vreg1 = STATEPOINT vreg1<tied-def0>).
foldMemoryOperand is called from spillAroundUses in InlineSpiller. And this method (besides few other optimizations) for every VReg use tries to fold register into memory operand or, if unsuccessful, inserts reload before instruction.
In some sense it turns VReg into StackSlot. And since Def of stack slot is invisible to LLVM (just like with old statepoint), it "magically" works.

But you're converting an instruction which defines a vreg to one which doesn't. If the original definition vreg had uses, isn't this a miscompile?

I can't wrap my head around why untieing operands would be correct here. It seems like we could end up with a use folded, but a def not leaving to invalid codegen? Can you either expand on the justification or discuss offline?

LGTM with one minor stylistic question. Can be addressed in separate change if desired.

Are two byte nops legal on all 32 bit x86? The comment seems to imply no.

LGTM w/minor change suggested.

Agreed with Eli, simply having two passes over operands would be much cleaner.

LGTM

commit 0e7c77053f560d391fecbec5d5e0e42082865c8a