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

LGTM w/required changes applied before commit. If you disagree on any point, further review needed and LGTM does not apply.

Let's discuss offline.

Please pull out a purely NFC patch without any of the first-N-vregs bits which is purely functional. (i.e. replaces the existing target node with the generic node, introduces the node type and wrapper class, but does nothing else.)

LGTM

The alternative here is not "do we relocate undef". It's do we let the backend chose arbitrary values for an undef operand. Said more strongly, the compiler is allowed to pick any value it wants for undef, and setting it to an arbitrary constant seems fine from a legality perspective.

Mostly minor comments on style to help drive review forward. This is a drive by comment, don't wait on me please.

Closed via commit 587fa99cfdb7d2a97143ba20ed8e8face57aa01c

In D80674#2059761, @dantrushin wrote:

LGTM with two notes:

• Does it makes sense to make GCArgs Optional<> too? Implementation might change later, but you won't need to change interface again.

At the moment, no. Or more accurately, I haven't thought about it enough yet to be comfortable revising the interface. We can always revise later if needed.

Closed w/commit 1af3705c7fe23db9d5308bfdf07bfbd04398b895

Make a deeper change to the interface to simplify the semantic description of the patch and avoid introducing something we'd have to fix later anyway.

In D80598#2056404, @anna wrote:

LGTM. thanks for working on this! Apart from dropping all of the extra handling code in RS4GC and verifier (which itself is a really good reason for this support btw), I'm assuming this for optimizations which know about the deopt_op bundle (or gc_transition bundle) after relocations are made explicit? i.e., a general robustness patch.

I can't quite parse what you meant with that last sentence.

Fix a couple of stylistic points noticed when looking at the web diff

Just to be clear, if the indices are the same, gc.relocates would already be removed right? i.e. the thing you're trying to solve is that the standpoint can have two operands which are the same value and thus two gc.relocates which are equivalent but not textually identical? Just want to make sure I understand the problem properly.

This patch bugged me when I first saw it go by; finally got back to looking at it today.

Has this been reverted? I see discussion of a fix being worked on, but has the change which triggered problems been reverted? I can't tell easily from the review history.