StackMap: Fix assertion on undef operands for anyregc

[arsenm]

Summary

There is some special handling of undef operands here which I don't
understand. For some reason a special offset value is used and the
type is changed to Constant, and then later was replaced with
ConstantIndex based on the dummy offset value. This also seems to be
inconsistently treated in the existing code. See the test changes in
statepoint-fixup-undef.mir since this stops the use of the constant
pool with the special offset in one of the run lines. I don't
understand why an undef operand would end up being encoded as anything
in the final binary.

Also have no idea if the new test checks are meaningful.

This avoids test failures in a future patch which starts discarding
liveness information on register allocation failures.

I also noticed we seem to be lacking verification for PATCHPOINT's
operand restrictions. In particular you hit assertions if the operand
for the number of operands isn't correct.

[dantrushin]

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)

[arsenm]

In 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 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)

[arsenm]

In D122582#3411978, @arsenm wrote:

In 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 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 is an alternative that avoids touching the undef operands

[dantrushin]

I'm sorry, I perhaps wasn't clear - English is not my native language. Let me try again.

First, StackMaps are used to communicate some (opaque to compiler) state to runtime.
Among other things, STATEPOINT passes set of pointers which need relocation to GC.
0xFEFEFEFE is a special sentinel value which means dead pointer. Now you just passing garbage to GC. (Theoretically this might be OK, practically I don't think so)

Second, current LLVM implementation supports passing pointers only in callee saved registers in StackMaps. If assigned by RA to other register, it must be spilled and that spill location must be recorded in StackMap instead.
This is what FixupStatepointCallerSaved pass does. Now you encode forbidden register in StackMap. Runtime will crash or behave strangely

Third, runtime might have enough knowledge of StackMap contents to wish to validate data types in it.
This is why 64-bit sentinel constant is used. And 64 bit constants are encoded via constant pool indexing. Now it will blow up.

PATCHPOINT is special pseudo with fancy encoding requirements. With anyregcc convention it requires function return value and arguments to be encoded in StackMap as well.
Obviously, it has different encoding requirements for THAT part.

I don't know if you really use PATHPOINT instruction or simply fixing some artificial test, but we use STATEPOINT instructions as described and you just broke it.

And, by the way, you test case does not make any sens to me. Is it correct to have a dead DEF with undef USE? What that would mean?

[arsenm]

In D122582#3414700, @dantrushin wrote:

0xFEFEFEFE is a special sentinel value which means dead pointer. Now you just passing garbage to GC. (Theoretically this might be OK, practically I don't think so)

It seems to me like you are blessing undef registers with special semantics, which is not what undef is for. If the value was undefined, it's undefined. The undef marker just informs the register liveness tracking of this. It doesn't make sense to have a lowering pass try to fix up the undefined value to something to avoid a crash. Transforming this is basically creating a mini-sanitizer/hardening.

PATCHPOINT is special pseudo with fancy encoding requirements. With anyregcc convention it requires function return value and arguments to be encoded in StackMap as well.
Obviously, it has different encoding requirements for THAT part.

I don't know if you really use PATHPOINT instruction or simply fixing some artificial test, but we use STATEPOINT instructions as described and you just broke it.

I don't use it and an simply trying to fix lit test failures after an unrelated patch.

And, by the way, you test case does not make any sens to me. Is it correct to have a dead DEF with undef USE? What that would mean?

Yes. These flags maintain liveness and don't imply any particular semantic meaning. It means there's no relation between the def and the use and the register is available between the two points. In the cases I'm trying to fix, the liveness is artificial. The register allocation failed, so the liveness information is essentially garbage. My patch will produce flags like this to avoid triggering verifier errors after fatal compile errors.

[dantrushin]

In D122582#3414852, @arsenm wrote:

In D122582#3414700, @dantrushin wrote:

0xFEFEFEFE is a special sentinel value which means dead pointer. Now you just passing garbage to GC. (Theoretically this might be OK, practically I don't think so)

It seems to me like you are blessing undef registers with special semantics, which is not what undef is for. If the value was undefined, it's undefined. The undef marker just informs the register liveness tracking of this. It doesn't make sense to have a lowering pass try to fix up the undefined value to something to avoid a crash. Transforming this is basically creating a mini-sanitizer/hardening.

OK, what about non-callee saved register appearing in stackmap, which is not supported?

[reames]

JFYI, the original special handling for undef was for diagnostic quality, nothing more. We wanted a fixed value for undef to avoid arbitrarily complex and broken stackmap entries when the input IR contained undef. Such stackmaps should be dynamically dead, but having to defer all validation until runtime was non-ideal. Using a constant value in place of undef gave us a way to constrain how broken the stackmap entries appeared. As a result, the JIT could meaningfully post-validate a stackmap section and detect errors eagerly.

(This is different than what Denis said above in an subtle way - the signal value does *not* mean "dead pointer". It's simply a constraint on the structure of a dynamically dead stackmap entry. Same in practice, slightly different in motivation.)

I think we can use any consistent handling for undef pointers in gc-live list. (Er, side note: structure of deopt is important, we can't "just drop" a field from it.)

I don't think anyone is using patchpoint. I suspect the semantics of it have bit rotten enough to be hard to make many reasonable assumptions. statepoint is in active use.

llvm/include/llvm/CodeGen/StackMaps.h

struct Location {
     LocationType Type = Unprocessed;
     unsigned Size = 0;
     unsigned Reg = 0;
     int64_t Offset = 0;
 
     Location() = default;
     Location(LocationType Type, unsigned Size, unsigned Reg, int64_t Offset)
         : Type(Type), Size(Size), Reg(Reg), Offset(Offset) {}
+
+    bool isUndefReg() const {
+      return Type == Constant && Reg == 0 && Offset == 0xFEFEFEFE;
+    }
+
+    static Location getUndefReg() {
+      return Location(Constant, sizeof(int64_t), 0, 0xFEFEFEFE);
+    }
   };
 
   struct LiveOutReg {
     unsigned short Reg = 0;
     unsigned short DwarfRegNum = 0;
     unsigned short Size = 0;
 
     LiveOutReg() = default;

llvm/lib/CodeGen/StackMaps.cpp

StackMaps::parseOperand(MachineInstr::const_mop_iterator MOI,
   // if it needs to.)
   if (MOI->isReg()) {
     // Skip implicit registers (this includes our scratch registers)
     if (MOI->isImplicit())
       return ++MOI;
 
     if (MOI->isUndef()) {
       // Record `undef` register as constant. Use same value as ISel uses.
-      Locs.emplace_back(Location::Constant, sizeof(int64_t), 0, 0xFEFEFEFE);
+      Locs.emplace_back(Location::getUndefReg());
       return ++MOI;
     }
 
     assert(Register::isPhysicalRegister(MOI->getReg()) &&
            "Virtreg operands should have been rewritten before now.");
     const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(MOI->getReg());
     assert(!MOI->getSubReg() && "Physical subreg still around.");
 
void StackMaps::recordStackMapOpers(const MCSymbol &MILabel,
   else
     while (MOI != MOE)
       MOI = parseOperand(MOI, MOE, Locations, LiveOuts);
 
   // Move large constants into the constant pool.
   for (auto &Loc : Locations) {
     // Constants are encoded as sign-extended integers.
     // -1 is directly encoded as .long 0xFFFFFFFF with no constant pool.
-    if (Loc.Type == Location::Constant && !isInt<32>(Loc.Offset)) {
+    if (Loc.Type == Location::Constant && !isInt<32>(Loc.Offset) &&
+        !Loc.isUndefReg()) {
       Loc.Type = Location::ConstantIndex;
       // ConstPool is intentionally a MapVector of 'uint64_t's (as
       // opposed to 'int64_t's).  We should never be in a situation
       // where we have to insert either the tombstone or the empty
       // keys into a map, and for a DenseMap<uint64_t, T> these are
       // (uint64_t)0 and (uint64_t)-1.  They can be and are
       // represented using 32 bit integers.
       assert((uint64_t)Loc.Offset != DenseMapInfo<uint64_t>::getEmptyKey() &&
recordStackMapOpers(L, MI, ID, MOI, MI.operands_end(),
                       opers.isAnyReg() && opers.hasDef());
 
 #ifndef NDEBUG
   // verify anyregcc
   auto &Locations = CSInfos.back().Locations;
   if (opers.isAnyReg()) {
     unsigned NArgs = opers.getNumCallArgs();
     for (unsigned i = 0, e = (opers.hasDef() ? NArgs + 1 : NArgs); i != e; ++i)
-      assert(Locations[i].Type == Location::Register &&
+      assert((Locations[i].Type == Location::Register ||
+              Locations[i].isUndefReg()) &&
              "anyreg arg must be in reg.");
   }
 #endif
 }
 
 void StackMaps::recordStatepoint(const MCSymbol &L, const MachineInstr &MI) {
   assert(MI.getOpcode() == TargetOpcode::STATEPOINT && "expected statepoint");
 

llvm/test/CodeGen/X86/stackmap-undef-operand-anyregcc.mir

+# RUN: llc -mtriple=x86_64-apple-darwin -start-after=virtregrewriter -o - %s | FileCheck %s
+
+# Check there's no assertion for anyregcc with an undef operand to a stackmap.
+
+# CHECK: __LLVM_StackMaps:
+# CHECK-NEXT: .byte 3
+# CHECK-NEXT: .byte 0
+# CHECK-NEXT: .short  0
+# CHECK-NEXT: .long 1
+# CHECK-NEXT: .long 0
+# CHECK-NEXT: .long 1
+# CHECK-NEXT: .quad _undef_anyregcc_patchpoint
+# CHECK-NEXT: .quad 8
+# CHECK-NEXT: .quad 1
+# CHECK-NEXT: .quad 12
+# CHECK-NEXT: .long Ltmp0-_undef_anyregcc_patchpoint
+# CHECK-NEXT: .short  0
+# CHECK-NEXT: .short  2
+# CHECK-NEXT: .byte 1
+# CHECK-NEXT: .byte 0
+# CHECK-NEXT: .short  8
+# CHECK-NEXT: .short  0
+# CHECK-NEXT: .short  0
+# CHECK-NEXT: .long 0
+# CHECK-NEXT: .byte 4
+# CHECK-NEXT: .byte 0
+# CHECK-NEXT: .short  8
+# CHECK-NEXT: .short  0
+# CHECK-NEXT: .short  0
+# CHECK-NEXT: .long 4278124286
+# CHECK-NEXT: .p2align  3
+# CHECK-NEXT: .short  0
+# CHECK-NEXT: .short  2
+# CHECK-NEXT: .short  0
+# CHECK-NEXT: .byte 0
+# CHECK-NEXT: .byte 8
+# CHECK-NEXT: .short  7
+# CHECK-NEXT: .byte 0
+# CHECK-NEXT: .byte 8
+# CHECK-NEXT: .p2align  3
+---
+name:  undef_anyregcc_patchpoint
+tracksRegLiveness: true
+frameInfo:
+  hasPatchPoint:   true
+  hasCalls:        true
+fixedStack:
+  - { id: 0, type: default, offset: 72, size: 8, alignment: 8, stack-id: default,
+      isImmutable: true, isAliased: false, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+body:             |
+  bb.0:
+    liveins: $rcx, $rdi, $rdx, $rsi, $r8, $r9
+
+    ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    dead renamable $rax = MOV64rm %fixed-stack.0, 1, $noreg, 0, $noreg :: (load (s64) from %fixed-stack.0)
+    renamable $rax = PATCHPOINT 12, 15, 0, 1, 13, undef renamable $rax, csr_64_allregs, implicit-def dead early-clobber $r11, implicit-def $rsp, implicit-def $ssp
+    ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    RET 0, $rax
+
+...

llvm/test/CodeGen/X86/statepoint-fixup-undef.mir

body:             |
   ; CHECK:   STATEPOINT 2, 5, 5, undef renamable $rax, $rdi, $esi, $edx, $rcx, killed $r8d, 2, 0, 2, 0, 2, 7, 1, 8, %stack.0, 0, 1, 8, %stack.1, 0, 1, 8, %stack.2, 0, 1, 8, %stack.3, 0, renamable $ebx, undef renamable $eax, 2, 6, 2, 0, 2, 0, 2, 0, csr_64, implicit-def $rsp, implicit-def $ssp, implicit killed $rbx :: (volatile load store (s64) on %stack.0), (volatile load store (s64) on %stack.1), (volatile load store (s64) on %stack.2), (volatile load store (s64) on %stack.3)
   ; CHECK:   ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
   ; CHECK:   RET 0
   ; STACKMAP-LABEL: __LLVM_StackMaps:
   ; STACKMAP: .byte 3
   ; STACKMAP: .byte 0
   ; STACKMAP: .short  0
   ; STACKMAP: .long 1
-  ; STACKMAP: .long 1
+  ; STACKMAP: .long 0
   ; STACKMAP: .long 1
   ; STACKMAP: .quad test_undef
   ; STACKMAP: .quad 88
   ; STACKMAP: .quad 1
-  ; STACKMAP: .quad 4278124286
   ; STACKMAP: .quad 2
   ; STACKMAP: .long .Ltmp0-test_undef
   ; STACKMAP: .short  0
   ; STACKMAP: .short  10
   ; STACKMAP: .byte 4
   ; STACKMAP: .byte 0
   ; STACKMAP: .short  8
   ; STACKMAP: .short  0
body:             |
   ; STACKMAP: .short  0
   ; STACKMAP: .long 8
   ; STACKMAP: .byte 1
   ; STACKMAP: .byte 0
   ; STACKMAP: .short  4
   ; STACKMAP: .short  3
   ; STACKMAP: .short  0
   ; STACKMAP: .long 0
-  ;      This is entry we're looking for, reference to constant pool entry 0xFEFEFEFE
-  ; STACKMAP: .byte 5
+  ;      This is entry we're looking for, reference to constant entry 0xFEFEFEFE
+  ; STACKMAP: .byte 4
   ; STACKMAP: .byte 0
   ; STACKMAP: .short  8
   ; STACKMAP: .short  0
   ; STACKMAP: .short  0
-  ; STACKMAP: .long 0
+  ; STACKMAP: .long 4278124286
   ; STACKMAP: .byte 4
   ; STACKMAP: .byte 0
   ; STACKMAP: .short  8
   ; STACKMAP: .short  0
   ; STACKMAP: .short  0
   ; STACKMAP: .long 6
   ; STACKMAP: .p2align  3
   ; STACKMAP: .short  0