[Statepoints] Use Indirect operands for spill slots

[reames]

Summary

Teach the statepoint lowering code to emit Indirect stackmap entries for spill inserted by StatepointLowering (i.e. SelectionDAG), but Direct stackmap entries for in-IR allocas which represent manual stack slots. This is what the docs call for (http://llvm.org/docs/StackMaps.html#stack-map-format), but we've been emitting both as Direct. This was pointed out recently on the mailing list as a bug. It also blocks http://reviews.llvm.org/D15632 which extends the lowering to handle vector-of-pointers since only Indirect references can encode a variable sized slot.

To implement this, I introduced a new flag on the StackObject class used to maintian information about stack slots. I original considered (and prototyped in http://reviews.llvm.org/D15632), the idea of using the existing isSpillSlot flag, but end up deciding that was a bit too risky and that the cost of adding a new flag was low. Having the new flag will also allow us - in the future - to emit better comments in verbose assembly which indicate where a particular stack spill around a call comes from. (deopt, gc, regalloc).

[atrick]

It's good that you use the right StackMap tags to be consistent with PatchPoints. Hopefully you can stop spilling your StatePoint values eventually.

[reames]

In D15759#316408, @atrick wrote:

Hopefully you can stop spilling your StatePoint values eventually.

This is still on the eventual todo list and some of the recent changes to the register allocator make it look a lot more straightforward than it used to be. Somewhat suprisingly, we've found this to be an utter non-issue performance wise, so it's hard to justify spending a lot of time on it. (The general issue is that any call which has hot spills is one we really should have inlined. Once we fix the inlining, which we'd want to do anyway, the spills disappear.)

llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h

struct StackObject {
     // entering the function and is not modified inside the function. By
     // default, fixed objects are immutable unless marked otherwise.
     bool isImmutable;
 
     // If true the stack object is used as spill slot. It
     // cannot alias any other memory objects.
     bool isSpillSlot;
 
+    /// If true, this stack slot is used to spill a value (could be deopt
+    /// and/or GC related) over a statepoint. We know that the address of the
+    /// slot can't alias any LLVM IR value.  This is very similiar to a Spill
+    /// Slot, but is created by statepoint lowering is SelectionDAG, not the
+    /// register allocator. 
+    bool isStatepointSpillSlot;
+
     /// If this stack object is originated from an Alloca instruction
     /// this value saves the original IR allocation. Can be NULL.
     const AllocaInst *Alloca;
 
     // If true, the object was mapped into the local frame
     // block and doesn't need additional handling for allocation beyond that.
     bool PreAllocated;
 
     // If true, an LLVM IR value might point to this object.
     // Normally, spill slots and fixed-offset objects don't alias IR-accessible
     // objects, but there are exceptions (on PowerPC, for example, some byval
     // arguments have ABI-prescribed offsets).
     bool isAliased;
 
     StackObject(uint64_t Sz, unsigned Al, int64_t SP, bool IM,
                 bool isSS, const AllocaInst *Val, bool A)
       : SPOffset(SP), Size(Sz), Alignment(Al), isImmutable(IM),
-        isSpillSlot(isSS), Alloca(Val), PreAllocated(false), isAliased(A) {}
+        isSpillSlot(isSS), isStatepointSpillSlot(false), Alloca(Val),
+        PreAllocated(false), isAliased(A) {}
   };
 
   /// The alignment of the stack.
   unsigned StackAlignment;
 
   /// Can the stack be realigned.
   /// Targets that set this to false don't have the ability to overalign
   /// their stack frame, and thus, overaligned allocas are all treated
public:
 
   /// Returns true if the specified index corresponds to a spill slot.
   bool isSpillSlotObjectIndex(int ObjectIdx) const {
     assert(unsigned(ObjectIdx+NumFixedObjects) < Objects.size() &&
            "Invalid Object Idx!");
     return Objects[ObjectIdx+NumFixedObjects].isSpillSlot;
   }
 
+  bool isStatepointSpillSlotObjectIndex(int ObjectIdx) const {
+    assert(unsigned(ObjectIdx+NumFixedObjects) < Objects.size() &&
+           "Invalid Object Idx!");
+    return Objects[ObjectIdx+NumFixedObjects].isStatepointSpillSlot;
+  }
+
   /// Returns true if the specified index corresponds to a dead object.
   bool isDeadObjectIndex(int ObjectIdx) const {
     assert(unsigned(ObjectIdx+NumFixedObjects) < Objects.size() &&
            "Invalid Object Idx!");
     return Objects[ObjectIdx+NumFixedObjects].Size == ~0ULL;
   }
 
   /// Returns true if the specified index corresponds to a variable sized
   /// object.
   bool isVariableSizedObjectIndex(int ObjectIdx) const {
     assert(unsigned(ObjectIdx + NumFixedObjects) < Objects.size() &&
            "Invalid Object Idx!");
     return Objects[ObjectIdx + NumFixedObjects].Size == 0;
   }
 
+  void markAsStatepointSpillSlotObjectIndex(int ObjectIdx) {
+    assert(unsigned(ObjectIdx+NumFixedObjects) < Objects.size() &&
+           "Invalid Object Idx!");
+    Objects[ObjectIdx+NumFixedObjects].isStatepointSpillSlot = true;
+    assert(isStatepointSpillSlotObjectIndex(ObjectIdx) && "inconsistent");
+  }
+
   /// Create a new statically sized stack object, returning
   /// a nonnegative identifier to represent it.
   int CreateStackObject(uint64_t Size, unsigned Alignment, bool isSS,
                         const AllocaInst *Alloca = nullptr);
 
   /// Create a new statically sized stack object that represents a spill slot,
   /// returning a nonnegative identifier to represent it.
   int CreateSpillStackObject(uint64_t Size, unsigned Alignment);

llvm/trunk/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "StatepointLowering.h"
 #include "SelectionDAGBuilder.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/CodeGen/FunctionLoweringInfo.h"
+#include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/GCMetadata.h"
 #include "llvm/CodeGen/GCStrategy.h"
 #include "llvm/CodeGen/SelectionDAG.h"
 #include "llvm/CodeGen/StackMaps.h"
 #include "llvm/IR/CallingConv.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
if (NextSlotToAllocate >= NumSlots) {
       assert(NextSlotToAllocate == NumSlots);
       // record stats
       if (NumSlots + 1 > StatepointMaxSlotsRequired) {
         StatepointMaxSlotsRequired = NumSlots + 1;
       }
 
       SDValue SpillSlot = Builder.DAG.CreateStackTemporary(ValueType);
       const unsigned FI = cast<FrameIndexSDNode>(SpillSlot)->getIndex();
+      auto *MFI = Builder.DAG.getMachineFunction().getFrameInfo();
+      MFI->markAsStatepointSpillSlotObjectIndex(FI);
+
       Builder.FuncInfo.StatepointStackSlots.push_back(FI);
       AllocatedStackSlots.push_back(true);
       return SpillSlot;
     }
     if (!AllocatedStackSlots[NextSlotToAllocate]) {
       const int FI = Builder.FuncInfo.StatepointStackSlots[NextSlotToAllocate];
       AllocatedStackSlots[NextSlotToAllocate] = true;
       return Builder.DAG.getFrameIndex(FI, ValueType);

llvm/trunk/lib/CodeGen/TargetLoweringBase.cpp

 }
 
 /// Replace/modify any TargetFrameIndex operands with a targte-dependent
 /// sequence of memory operands that is recognized by PrologEpilogInserter.
 MachineBasicBlock*
 TargetLoweringBase::emitPatchPoint(MachineInstr *MI,
                                    MachineBasicBlock *MBB) const {
   MachineFunction &MF = *MI->getParent()->getParent();
+  MachineFrameInfo &MFI = *MF.getFrameInfo();
+
+  // We're handling multiple types of operands here:
+  // PATCHPOINT MetaArgs - live-in, read only, direct
+  // STATEPOINT Deopt Spill - live-through, read only, indirect
+  // STATEPOINT Deopt Alloca - live-through, read only, direct
+  // (We're currently conservative and mark the deopt slots read/write in
+  // practice.) 
+  // STATEPOINT GC Spill - live-through, read/write, indirect
+  // STATEPOINT GC Alloca - live-through, read/write, direct
+  // The live-in vs live-through is handled already (the live through ones are
+  // all stack slots), but we need to handle the different type of stackmap
+  // operands and memory effects here.
 
   // MI changes inside this loop as we grow operands.
   for(unsigned OperIdx = 0; OperIdx != MI->getNumOperands(); ++OperIdx) {
     MachineOperand &MO = MI->getOperand(OperIdx);
     if (!MO.isFI())
       continue;
 
     // foldMemoryOperand builds a new MI after replacing a single FI operand
     // with the canonical set of five x86 addressing-mode operands.
     int FI = MO.getIndex();
     MachineInstrBuilder MIB = BuildMI(MF, MI->getDebugLoc(), MI->getDesc());
 
     // Copy operands before the frame-index.
     for (unsigned i = 0; i < OperIdx; ++i)
       MIB.addOperand(MI->getOperand(i));
-    // Add frame index operands: direct-mem-ref tag, #FI, offset.
+    // Add frame index operands recognized by stackmaps.cpp
+    if (MFI.isStatepointSpillSlotObjectIndex(FI)) {
+      // indirect-mem-ref tag, size, #FI, offset.
+      // Used for spills inserted by StatepointLowering.  This codepath is not
+      // used for patchpoints/stackmaps at all, for these spilling is done via
+      // foldMemoryOperand callback only.
+      assert(MI->getOpcode() == TargetOpcode::STATEPOINT && "sanity");
+      MIB.addImm(StackMaps::IndirectMemRefOp);
+      MIB.addImm(MFI.getObjectSize(FI));
+      MIB.addOperand(MI->getOperand(OperIdx));
+      MIB.addImm(0);
+    } else {
+      // direct-mem-ref tag, #FI, offset.
+      // Used by patchpoint, and direct alloca arguments to statepoints
       MIB.addImm(StackMaps::DirectMemRefOp);
       MIB.addOperand(MI->getOperand(OperIdx));
       MIB.addImm(0);
+    }
     // Copy the operands after the frame index.
     for (unsigned i = OperIdx + 1; i != MI->getNumOperands(); ++i)
       MIB.addOperand(MI->getOperand(i));
 
     // Inherit previous memory operands.
     MIB->setMemRefs(MI->memoperands_begin(), MI->memoperands_end());
     assert(MIB->mayLoad() && "Folded a stackmap use to a non-load!");
 
     // Add a new memory operand for this FI.
-    const MachineFrameInfo &MFI = *MF.getFrameInfo();
     assert(MFI.getObjectOffset(FI) != -1);
 
     unsigned Flags = MachineMemOperand::MOLoad;
     if (MI->getOpcode() == TargetOpcode::STATEPOINT) {
       Flags |= MachineMemOperand::MOStore;
       Flags |= MachineMemOperand::MOVolatile;
     }
     MachineMemOperand *MMO = MF.getMachineMemOperand(

llvm/trunk/test/CodeGen/X86/statepoint-stackmap-format.ll

 ; RUN: llc < %s -mtriple="x86_64-pc-linux-gnu" | FileCheck %s
 ; RUN: llc < %s -mtriple="x86_64-pc-unknown-elf" | FileCheck %s
 
 ; This test is a sanity check to ensure statepoints are generating StackMap
 ; sections correctly.  This is not intended to be a rigorous test of the 
 ; StackMap format (see the stackmap tests for that).
 
 target datalayout = "e-i64:64-f80:128-n8:16:32:64-S128"
 
 declare zeroext i1 @return_i1()
 
 define i1 @test(i32 addrspace(1)* %ptr_base, i32 %arg)
   gc "statepoint-example" {
-; CHECK-LABEL: test
+; CHECK-LABEL: test:
 ; Do we see two spills for the local values and the store to the
 ; alloca?
 ; CHECK: subq $40, %rsp
 ; CHECK: movq $0,   24(%rsp)
 ; CHECK: movq %rdi, 16(%rsp)
 ; CHECK: movq %rax, 8(%rsp)
 ; CHECK: callq return_i1
 ; CHECK: addq $40, %rsp

 ; Num Callsites
 ; CHECK-NEXT:   .long 3
 
 ; Functions and stack size
 ; CHECK-NEXT:   .quad test
 ; CHECK-NEXT:   .quad 40
 ; CHECK-NEXT:   .quad test_derived_arg
 ; CHECK-NEXT:   .quad 40
+; CHECK-NEXT:   .quad test_id
+; CHECK-NEXT:   .quad 8
 
 ;
 ; test
 ;
 
-; Large Constants
-; Statepoint ID only
-; CHECK: .quad  0
+; Statepoint ID
+; CHECK-NEXT: .quad 0
 
 ; Callsites
 ; Constant arguments
-; CHECK: .long  .Ltmp1-test
+; CHECK-NEXT: .long .Ltmp1-test
 ; CHECK: .short 0
 ; CHECK: .short 11
 ; SmallConstant (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
 ; SmallConstant (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
 ; SmallConstant (2)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  2
-; Direct Spill Slot [RSP+0]
-; CHECK: .byte  2
+; Indirect Spill Slot [RSP+0]
+; CHECK: .byte  3
 ; CHECK: .byte  8
 ; CHECK: .short 7
 ; CHECK: .long  16
 ; SmallConstant  (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
 ; SmallConstant  (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
 ; SmallConstant  (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
-; Direct Spill Slot [RSP+16]
-; CHECK: .byte  2
+; Indirect Spill Slot [RSP+16]
+; CHECK: .byte  3
 ; CHECK: .byte  8
 ; CHECK: .short 7
 ; CHECK: .long  16
-; Direct Spill Slot [RSP+8]
-; CHECK: .byte  2
+; Indirect Spill Slot [RSP+8]
+; CHECK: .byte  3
 ; CHECK: .byte  8
 ; CHECK: .short 7
 ; CHECK: .long  8
-; Direct Spill Slot [RSP+16]
-; CHECK: .byte  2
+; Indirect Spill Slot [RSP+16]
+; CHECK: .byte  3
 ; CHECK: .byte  8
 ; CHECK: .short 7
 ; CHECK: .long  16
-; Direct Spill Slot [RSP+16]
-; CHECK: .byte  2
+; Indirect Spill Slot [RSP+16]
+; CHECK: .byte  3
 ; CHECK: .byte  8
 ; CHECK: .short 7
 ; CHECK: .long  16
 
 ; No Padding or LiveOuts
 ; CHECK: .short 0
 ; CHECK: .short 0
 ; CHECK: .align 8
 
 ;
 ; test_derived_arg
-;
 
-; Large Constants
-; Statepoint ID only
-; CHECK: .quad  0
+; Statepoint ID
+; CHECK-NEXT: .quad 0
 
 ; Callsites
 ; Constant arguments
-; CHECK: .long  .Ltmp3-test_derived_arg
+; CHECK-NEXT: .long .Ltmp3-test_derived_arg
 ; CHECK: .short 0
 ; CHECK: .short 11
 ; SmallConstant (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
 ; SmallConstant (2)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  2
-; Direct Spill Slot [RSP+0]
-; CHECK: .byte  2
+; Indirect Spill Slot [RSP+0]
+; CHECK: .byte  3
 ; CHECK: .byte  8
 ; CHECK: .short 7
 ; CHECK: .long  16
 ; SmallConstant  (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
 ; SmallConstant  (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
 ; SmallConstant  (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
-; Direct Spill Slot [RSP+16]
-; CHECK: .byte  2
+; Indirect Spill Slot [RSP+16]
+; CHECK: .byte  3
 ; CHECK: .byte  8
 ; CHECK: .short 7
 ; CHECK: .long  16
-; Direct Spill Slot [RSP+8]
-; CHECK: .byte  2
+; Indirect Spill Slot [RSP+8]
+; CHECK: .byte  3
 ; CHECK: .byte  8
 ; CHECK: .short 7
 ; CHECK: .long  8
-; Direct Spill Slot [RSP+16]
-; CHECK: .byte  2
+; Indirect Spill Slot [RSP+16]
+; CHECK: .byte  3
 ; CHECK: .byte  8
 ; CHECK: .short 7
 ; CHECK: .long  16
-; Direct Spill Slot [RSP+16]
-; CHECK: .byte  2
+; Indirect Spill Slot [RSP+16]
+; CHECK: .byte  3
 ; CHECK: .byte  8
 ; CHECK: .short 7
 ; CHECK: .long  16
 
 ; No Padding or LiveOuts
 ; CHECK: .short 0
 ; CHECK: .short 0
 ; CHECK: .align 8
 
 ; Records for the test_id function:
-; No large constants
 
 ; The Statepoint ID:
-; CHECK: .quad  237
+; CHECK-NEXT: .quad 237
 
 ; Instruction Offset
-; CHECK: .long  .Ltmp5-test_id
+; CHECK-NEXT: .long .Ltmp5-test_id
 
 ; Reserved:
 ; CHECK: .short 0
 
 ; NumLocations:
 ; CHECK: .short 3
 
 ; StkMapRecord[0]: