[PATCH] D125680: Correctly legalise stackmap operands

Wed May 18 07:49:23 PDT 2022

dantrushin added a comment.

In D125680#3522346 <https://reviews.llvm.org/D125680#3522346>, @vext01 wrote:

>> Please do not touch statepoint in this patch. If you do not really need it, leave it alone at all.
>
> Just to clarify why I've proposed this:
>
> `llvm.experimental.stackmap`, `llvm.experimental.patchpoint` and `llvm.experimental.gc.statepoint` used to all share the same DAG building code via a function called `addStackMapLiveVars()` in `SelectionDAGBuilder.cpp`.

No, Statepoint does not use this function.

> This function incorrectly emits target nodes, thus erroneously side-stepping the legalisation stage.

> My change (so-far) detaches only `llvm.experimental.stackmap` from this function.

This is enough for a _single_ patch
You can (and better) do patchpoint in a separate patch.
Statepoint is much more complicated and is in active productionuse today. It is too easy to break, so we will need to negotiate first if you'll want to change it

> Ideally, all three facilities would use a common function that emits target-independent nodes when legalisation is required, but if upstream is against that, we should at the very least add a big flashing neon message to the effect of:
>
>   // XXX: this is incorrect because it emits target nodes, meaning that operands do not get legalised.

For statepoint deopt arguments are always legal right now (as well as gc live pointers), so they do not need any special processing.

> Are we on the same page?

As far as I know nobody cares about stackmap/patchpoint. Neither do we.
But as I said, STATEPOINT is very important for us in its current form.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125680/new/

https://reviews.llvm.org/D125680