[PATCH] D122582: StackMap: Fix assertion on undef operands for anyregc

Mon Mar 28 11:47:33 PDT 2022

arsenm added a comment.

In D122582#3411978 <https://reviews.llvm.org/D122582#3411978>, @arsenm wrote:

> In D122582#3411580 <https://reviews.llvm.org/D122582#3411580>, @dantrushin wrote:
>
>> StackMap contents is opaque for compiler and is interpreted by runtime. With your change, relocating garbage collector can get some garbage in register and try to relocate (move) it. This will result in immediate crash.
>> Also, StackMap can encode operand types in it. Suddenly changing operand size from `.quad` to `.long` might also break runtime's invariants.
>>
>> I think proper fix would be to add  parameter to `StackMaps::parseOperand()` which will tell if replacement of undef is legal or not. For call arguments is should be `false` while for variable part it must be `true`.
>> (And then `StackMaps::recordPatchPoint()` in case of `anyreg` convention will have to split call to `recordStackMapOpers` into two calls - one for call arguments and second for variable arguments)
>
> I tried this and am getting confused about the operand structure for PATCHPOINT and why there need to be 2 calls. It seems like it's already splitting the set of operands since getStackMapStartIdx is different for anyregcc.
>
> I keep coming back to questioning why do this in the first place. i.e. the original change in D94703 <https://reviews.llvm.org/D94703> doesn't make any sense to me. If the motivation is purely to avoid the verifier error, the correct solution is to handle undef in later passes to preserve correct liveness. From a lowering perspective, undef doesn't exist and shouldn't receive special treatment. If I revert that patch, the verifier error is introduced in FixupStatepointCallerSaved, which blindly inserts spills. It could instead skip the spill for undef registers (or would need to propagate the undef on the new store value operand)

D122605 <https://reviews.llvm.org/D122605> is an alternative that avoids touching the undef operands

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

https://reviews.llvm.org/D122582