[StatepointLowering] Handle UNDEF gc values.

[dantrushin]

Summary

Do not spill UNDEF GC values. Instead, replace corresponding
gc.relocate intrinsic with an (arbitrary, but recognizable) constant.

[dantrushin]

llvm/include/llvm/CodeGen/GCStrategy.h

77 ↗

(On Diff #266827)

Should this be spelled NeedsSafePoints (needs vs. needed) ?

[reames]

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.

There's a potential *performance* implication of choosing a particular poison pattern instead of letting it be whatever value happened to last be in a register, but given an undef appearing in the relocation list strongly indicates the value either isn't used or we're in dead code, I don't think this really matters. Particular since instcombine explicitly removes such gc.relocates.

(I tried to prototype the "better codegen" option here and promptly got crashes in the backend. These aren't worth investigating. Picking a constant is clearly better than spilling so let's just take the incremental step forward.)

Please update the patch to make this behaviour unconditional. We can be fancier if we ever see a perf impact here.

[dantrushin]

Updated patch to handle UNDEF unconditionally.

By the way, I was following this comment in InstCombineCalls.cpp:

// Undef is undef, even after relocation.
// TODO: provide a hook for this in GCStrategy.  This is clearly legal for
// most practical collectors, but there was discussion in the review thread
// about whether it was legal for all possible collectors.

Is it not relevant anymore?

[reames]

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

Also, make sure you update your commit message before landing. The current description is out of sync with the patch.

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

232

Use Incomng.isUndef(), add to list in following return since the comment applies as well.

394

Please remove the assert by folding it into an additional clause in the condition.

e.g. if (Incoming is undef and <= 64 bits)

Do the same below.

Vector undefs are legal and shouldn't crash.

398

Add to comment. (This is legal since the compiler is always allowed to chose an arbitrary value for undef.)

1018

Use SD.isUndef

1022

After removing the assert, replace comment with:
Lowering relocate(undef) as arbitrary constant. Current constant value is chosen such that it's unlikely to be a valid pointer.

[dantrushin]

Update according to comments.

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

 /// statepoint spilling.  If we can find a spill slot for the incoming value,
 /// mark that slot as allocated, and reuse the same slot for this safepoint.
 /// This helps to avoid series of loads and stores that only serve to reshuffle
 /// values on the stack between calls.
 static void reservePreviousStackSlotForValue(const Value *IncomingValue,
                                              SelectionDAGBuilder &Builder) {
   SDValue Incoming = Builder.getValue(IncomingValue);
 
   if (isa<ConstantSDNode>(Incoming) || isa<ConstantFPSDNode>(Incoming) ||
-      isa<FrameIndexSDNode>(Incoming)) {
+      isa<FrameIndexSDNode>(Incoming) || Incoming.isUndef()) {
     // We won't need to spill this, so no need to check for previously
     // allocated stack slots
     return;
   }
 
   SDValue OldLocation = Builder.StatepointLowering.getLocation(Incoming);
   if (OldLocation.getNode())
     // Duplicates in input
lowerIncomingStatepointValue(SDValue Incoming, bool RequireSpillSlot,
                              SmallVectorImpl<SDValue> &Ops,
                              SmallVectorImpl<MachineMemOperand *> &MemRefs,
                              SelectionDAGBuilder &Builder) {
   // Note: We know all of these spills are independent, but don't bother to
   // exploit that chain wise.  DAGCombine will happily do so as needed, so
   // doing it here would be a small compile time win at most.
   SDValue Chain = Builder.getRoot();
 
+  if (Incoming.isUndef() && Incoming.getValueType().getSizeInBits() <= 64) {
+    // Put an easily recognized constant that's unlikely to be a valid
+    // value so that uses of undef by the consumer of the stackmap is
+    // easily recognized. This is legal since the compiler is always
+    // allowed to chose an arbitrary value for undef.
+    pushStackMapConstant(Ops, Builder, 0xFEFEFEFE);
+    return;
+  }
+
   // If the original value was a constant, make sure it gets recorded as
   // such in the stackmap.  This is required so that the consumer can
   // parse any internal format to the deopt state.  It also handles null
   // pointers and other constant pointers in GC states.  Note the constant
   // vectors do not appear to actually hit this path and that anything larger
   // than an i64 value (not type!) will fail asserts here.
   if (ConstantFPSDNode *C = dyn_cast<ConstantFPSDNode>(Incoming)) {
     pushStackMapConstant(Ops, Builder,
#ifndef NDEBUG
   auto *Ty = Relocate.getType()->getScalarType();
   if (auto IsManaged = GFI->getStrategy().isGCManagedPointer(Ty))
     assert(*IsManaged && "Non gc managed pointer relocated!");
 #endif
 
   const Value *DerivedPtr = Relocate.getDerivedPtr();
   SDValue SD = getValue(DerivedPtr);
 
+  if (SD.isUndef() && SD.getValueType().getSizeInBits() <= 64) {
+    // Lowering relocate(undef) as arbitrary constant. Current constant value
+    // is chosen such that it's unlikely to be a valid pointer.
+    setValue(&Relocate, DAG.getTargetConstant(0xFEFEFEFE, SDLoc(SD), MVT::i64));
+    return;
+  }
+
   auto &SpillMap = FuncInfo.StatepointSpillMaps[Relocate.getStatepoint()];
   auto SlotIt = SpillMap.find(DerivedPtr);
   assert(SlotIt != SpillMap.end() && "Relocating not lowered gc value");
   Optional<int> DerivedPtrLocation = SlotIt->second;
 
   // We didn't need to spill these special cases (constants and allocas).
   // See the handling in spillIncomingValueForStatepoint for detail.
   if (!DerivedPtrLocation) {

llvm/test/CodeGen/X86/statepoint-uniqueing.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -verify-machineinstrs < %s | FileCheck %s
 
 target datalayout = "e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-linux-gnu"
 
 declare void @use(...)
 declare void @f()
 declare token @llvm.experimental.gc.statepoint.p0f_isVoidf(i64, i32, void ()*, i32, i32, ...)
 declare i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token, i32, i32) #3
 declare i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token, i32, i32) #3
 
 ;; Two gc.relocates of the same input, should require only a single spill/fill
 define void @test_gcrelocate_uniqueing(i32 addrspace(1)* %ptr) gc "statepoint-example" {
 ; CHECK-LABEL: test_gcrelocate_uniqueing:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    subq $24, %rsp
-; CHECK-NEXT:    .cfi_def_cfa_offset 32
-; CHECK-NEXT:    movq %rdi, {{[0-9]+}}(%rsp)
+; CHECK-NEXT:    pushq %rax
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    movq %rdi, (%rsp)
 ; CHECK-NEXT:    callq f
 ; CHECK-NEXT:  .Ltmp0:
-; CHECK-NEXT:    movq {{[0-9]+}}(%rsp), %rdi
+; CHECK-NEXT:    movq (%rsp), %rdi
 ; CHECK-NEXT:    movq %rdi, %rsi
 ; CHECK-NEXT:    xorl %eax, %eax
 ; CHECK-NEXT:    callq use
-; CHECK-NEXT:    addq $24, %rsp
+; CHECK-NEXT:    popq %rax
 ; CHECK-NEXT:    .cfi_def_cfa_offset 8
 ; CHECK-NEXT:    retq
   %tok = tail call token (i64, i32, void ()*, i32, i32, ...)
       @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @f, i32 0, i32 0, i32 0, i32 0, i32 addrspace(1)* %ptr, i32 addrspace(1)* %ptr) ["deopt" (i32 addrspace(1)* %ptr, i32 undef)]
   %a = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token %tok, i32 7, i32 7)
   %b = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token %tok, i32 8, i32 8)
   call void (...) @use(i32 addrspace(1)* %a, i32 addrspace(1)* %b)
   ret void
 }
 
 ;; Two gc.relocates of a bitcasted pointer should only require a single spill/fill
 define void @test_gcptr_uniqueing(i32 addrspace(1)* %ptr) gc "statepoint-example" {
 ; CHECK-LABEL: test_gcptr_uniqueing:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    subq $24, %rsp
-; CHECK-NEXT:    .cfi_def_cfa_offset 32
-; CHECK-NEXT:    movq %rdi, {{[0-9]+}}(%rsp)
+; CHECK-NEXT:    pushq %rax
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    movq %rdi, (%rsp)
 ; CHECK-NEXT:    callq f
 ; CHECK-NEXT:  .Ltmp1:
-; CHECK-NEXT:    movq {{[0-9]+}}(%rsp), %rdi
+; CHECK-NEXT:    movq (%rsp), %rdi
 ; CHECK-NEXT:    movq %rdi, %rsi
 ; CHECK-NEXT:    xorl %eax, %eax
 ; CHECK-NEXT:    callq use
-; CHECK-NEXT:    addq $24, %rsp
+; CHECK-NEXT:    popq %rax
 ; CHECK-NEXT:    .cfi_def_cfa_offset 8
 ; CHECK-NEXT:    retq
   %ptr2 = bitcast i32 addrspace(1)* %ptr to i8 addrspace(1)*
   %tok = tail call token (i64, i32, void ()*, i32, i32, ...)
       @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @f, i32 0, i32 0, i32 0, i32 0, i32 addrspace(1)* %ptr, i8 addrspace(1)* %ptr2) ["deopt" (i32 addrspace(1)* %ptr, i32 undef)]
   %a = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token %tok, i32 7, i32 7)
   %b = call i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token %tok, i32 8, i32 8)
   call void (...) @use(i32 addrspace(1)* %a, i8 addrspace(1)* %b)
   ret void
 }
 
 ;; A GC value is not dead, and does need to be spill (but not filed) if
 ;; that same value is also in the deopt list.
 define void @test_deopt_use(i32 addrspace(1)* %ptr) gc "statepoint-example" {
 ; CHECK-LABEL: test_deopt_use:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    subq $24, %rsp
-; CHECK-NEXT:    .cfi_def_cfa_offset 32
-; CHECK-NEXT:    movq %rdi, {{[0-9]+}}(%rsp)
+; CHECK-NEXT:    pushq %rax
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    movq %rdi, (%rsp)
 ; CHECK-NEXT:    callq f
 ; CHECK-NEXT:  .Ltmp2:
-; CHECK-NEXT:    addq $24, %rsp
+; CHECK-NEXT:    popq %rax
 ; CHECK-NEXT:    .cfi_def_cfa_offset 8
 ; CHECK-NEXT:    retq
   tail call token (i64, i32, void ()*, i32, i32, ...)
       @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @f, i32 0, i32 0, i32 0, i32 0, i32 addrspace(1)* %ptr) ["deopt" (i32 addrspace(1)* %ptr, i32 undef)]
   ret void
 }
 
 ;; A GC value which is truely unused does not need to spilled or filled.