[WIP] Improve deopt spilling for arguments passed on the stack

[reames]

Summary

This patch is not likely to get committed in it's current form; I got most of the way done and discovered a better approach.

The idea behind this was that we are generating new spill slots for deopt values which are arguments that were passed on the stack. This is unnecessary. This patch extends the SelectionDAGBuilder logic around statepoints to handle specifically this case.

This does help our existing lowering, but the main reason was to avoid a serious regression in an upcoming patch around live-in-deopt semantics. However, I discovered right before posting this that the peepholeopt MI pass should be catching these cases and isn't. The right fix is to implement the live in piece and then fix peepholeopt.

Worth noting is that this patch improves lowering for live-through deopt. If it turns out live-on-return is harder than it looks, this might be a good backup position. In particular, I'm trying to get both live-in and live-on-return implemented; I have a strategy for live-in that looks workable, but live-on-return is still questionable. If I can't get live-on-return, I'll probably want to revisit this patch.

[reames]

Superseeded by https://reviews.llvm.org/D24103

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

StatepointLoweringState::allocateStackSlot(EVT ValueType,
   Builder.FuncInfo.StatepointStackSlots.push_back(FI);
 
   StatepointMaxSlotsRequired = std::max<unsigned long>(
       StatepointMaxSlotsRequired, Builder.FuncInfo.StatepointStackSlots.size());
 
   return SpillSlot;
 }
 
+/// There may be an existing slot which can be used.  In particular, for
+/// readonly uses (deopt) we can reuse space associated with argument passing
+/// since we know we won't modify them within the callee (this function).
+static SDValue getFixedSlot(SDValue Incoming, bool ReadOnly,
+                            SelectionDAGBuilder &Builder) {
+  // We could potentially consider letting the GC rewrite these slots, but
+  // doing so might break assumptions elsewhere in the backend.  This needs to
+  // be evaluated carefully.
+  if (!ReadOnly)
+    return SDValue();
+
+  // TODO: We could also consider arguments pushed on the stack for this call.
+  // In priciple, this could be fine, but we need to review the ABI carefully
+  // to be sure if the immutability of arguments is a ABI guarantee or an LLVM
+  // implementation accident.
+
+  // Look to see if we were passed a deopt value in an argument that spilled to
+  // the stack by our caller.  If so, we've got the perfect slot to record
+  // since it already contains the value we need.
+  auto *LNode = dyn_cast<LoadSDNode>(Incoming);
+  if (!LNode)
+    return SDValue();
+  if (LNode->getExtensionType() != ISD::NON_EXTLOAD ||
+      LNode->isIndexed())
+    return SDValue();
+  assert(LNode->getOffset().isUndef() && "Shouldn't be indexed!");
+  auto *FINode = dyn_cast<FrameIndexSDNode>(LNode->getBasePtr().getNode());
+  if (!FINode)
+    return SDValue();
+  const int FrameIdx = FINode->getIndex();
+  auto *MFI = Builder.DAG.getMachineFunction().getFrameInfo();
+  if (!MFI->isImmutableObjectIndex(FrameIdx))
+    return SDValue();
+
+  // This is a bit of a hack.  We need to record the fact that this stack slot
+  // is a statepoint spill slot so that our target frame index gets interpreted
+  // as an Indirect stackmap entry, not a direct one.
+  MFI->markAsStatepointSpillSlotObjectIndex(FrameIdx);
+    
+  // We use TargetFrameIndex so that isel will not select it into LEA
+  return Builder.DAG.getTargetFrameIndex(FrameIdx, Incoming.getValueType());
+}
+
 /// Utility function for reservePreviousStackSlotForValue. Tries to find
 /// stack slot index to which we have spilled value for previous statepoints.
 /// LookUpDepth specifies maximum DFS depth this function is allowed to look.
 static Optional<int> findPreviousSpillSlot(const Value *Val,
                                            SelectionDAGBuilder &Builder,
                                            int LookUpDepth) {
   // Can not look any further - give up now
   if (LookUpDepth <= 0)

 }
 
 /// Try to find existing copies of the incoming values in stack slots used for
 /// 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,
+                                             bool ReadOnly, 
                                              SelectionDAGBuilder &Builder) {
 
   SDValue Incoming = Builder.getValue(IncomingValue);
 
-  if (isa<ConstantSDNode>(Incoming) || isa<FrameIndexSDNode>(Incoming)) {
+  if (isa<ConstantSDNode>(Incoming) || isa<FrameIndexSDNode>(Incoming) ||
+      getFixedSlot(Incoming, ReadOnly, Builder)) {
     // 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
#endif
   assert(Loc.getNode());
   return std::make_pair(Loc, Chain);
 }
 
 /// Lower a single value incoming to a statepoint node.  This value can be
 /// either a deopt value or a gc value, the handling is the same.  We special
 /// case constants and allocas, then fall back to spilling if required.
 static void lowerIncomingStatepointValue(SDValue Incoming,
+                                         bool ReadOnly,
                                          SmallVectorImpl<SDValue> &Ops,
                                          SelectionDAGBuilder &Builder) {
   SDValue Chain = Builder.getRoot();
 
   if (ConstantSDNode *C = dyn_cast<ConstantSDNode>(Incoming)) {
     // 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.
     pushStackMapConstant(Ops, Builder, C->getSExtValue());
   } else if (FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Incoming)) {
     // This handles allocas as arguments to the statepoint (this is only
     // really meaningful for a deopt value.  For GC, we'd be trying to
     // relocate the address of the alloca itself?)
     Ops.push_back(Builder.DAG.getTargetFrameIndex(FI->getIndex(),
                                                   Incoming.getValueType()));
+  } else if (SDValue Slot = getFixedSlot(Incoming, ReadOnly, Builder)) {
+    Ops.push_back(Slot);
   } else {
     // Otherwise, locate a spill slot and explicitly spill it so it
     // can be found by the runtime later.  We currently do not support
     // tracking values through callee saved registers to their eventual
     // spill location.  This would be a useful optimization, but would
     // need to be optional since it requires a lot of complexity on the
     // runtime side which not all would support.
     auto Res = spillIncomingStatepointValue(Incoming, Chain, Builder);

 #endif
 
   // Before we actually start lowering (and allocating spill slots for values),
   // reserve any stack slots which we judge to be profitable to reuse for a
   // particular value.  This is purely an optimization over the code below and
   // doesn't change semantics at all.  It is important for performance that we
   // reserve slots for both deopt and gc values before lowering either.
   for (const Value *V : SI.DeoptState) {
-    reservePreviousStackSlotForValue(V, Builder);
+    reservePreviousStackSlotForValue(V, true, Builder);
   }
   for (unsigned i = 0; i < SI.Bases.size(); ++i) {
-    reservePreviousStackSlotForValue(SI.Bases[i], Builder);
-    reservePreviousStackSlotForValue(SI.Ptrs[i], Builder);
+    reservePreviousStackSlotForValue(SI.Bases[i], false, Builder);
+    reservePreviousStackSlotForValue(SI.Ptrs[i], false, Builder);
   }
 
   // First, prefix the list with the number of unique values to be
   // lowered.  Note that this is the number of *Values* not the
   // number of SDValues required to lower them.
   const int NumVMSArgs = SI.DeoptState.size();
   pushStackMapConstant(Ops, Builder, NumVMSArgs);
 
   // The vm state arguments are lowered in an opaque manner.  We do not know
   // what type of values are contained within.
   for (const Value *V : SI.DeoptState) {
     SDValue Incoming = Builder.getValue(V);
-    lowerIncomingStatepointValue(Incoming, Ops, Builder);
+    lowerIncomingStatepointValue(Incoming, true, Ops, Builder);
   }
 
   // Finally, go ahead and lower all the gc arguments.  There's no prefixed
   // length for this one.  After lowering, we'll have the base and pointer
   // arrays interwoven with each (lowered) base pointer immediately followed by
   // it's (lowered) derived pointer.  i.e
   // (base[0], ptr[0], base[1], ptr[1], ...)
   for (unsigned i = 0; i < SI.Bases.size(); ++i) {
     const Value *Base = SI.Bases[i];
-    lowerIncomingStatepointValue(Builder.getValue(Base), Ops, Builder);
+    lowerIncomingStatepointValue(Builder.getValue(Base), false, Ops, Builder);
 
     const Value *Ptr = SI.Ptrs[i];
-    lowerIncomingStatepointValue(Builder.getValue(Ptr), Ops, Builder);
+    lowerIncomingStatepointValue(Builder.getValue(Ptr), false, Ops, Builder);
   }
 
   // If there are any explicit spill slots passed to the statepoint, record
   // them, but otherwise do not do anything special.  These are user provided
   // allocas and give control over placement to the consumer.  In this case,
   // it is the contents of the slot which may get updated, not the pointer to
   // the alloca
   for (Value *V : SI.GCArgs) {

test/CodeGen/X86/statepoint-fixed-slots.ll

+; RUN: llc -O3 < %s | FileCheck %s
+target datalayout = "e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+declare void @foo()
+
+declare void @use(...)
+
+define void @test(i32 %v, i32 %v2, i32 %v3, i32 %v4, i32 %v5, i32 %v6, i32 %v7, i32 %v8, i32 %v9, i32 %v10) gc "statepoint-example" {
+; CHECK-LABEL: test
+; Looking for the fact that the last 4 arguments are not loaded, and respilled in diffect locations
+; CHECK: subq $24, %rsp
+; CHECK: movl %edi, 20(%rsp)
+; CHECK: movl %esi, 16(%rsp)
+; CHECK: movl %edx, 12(%rsp)
+; CHECK: movl %ecx, 8(%rsp)
+; CHECK: movl %r8d, 4(%rsp)
+; CHECK: movl %r9d, (%rsp)
+; CHECK: callq  foo
+
+  %statepoint_token = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 2882400000, i32 0, void ()* @foo, i32 0, i32 0, i32 0, i32 10, i32 %v, i32 %v2, i32 %v3, i32 %v4, i32 %v5, i32 %v6, i32 %v7, i32 %v8, i32 %v9, i32 %v10)
+  ret void
+}
+
+declare token @llvm.experimental.gc.statepoint.p0f_isVoidf(i64, i32, void ()*, i32, i32, ...)