Split the calling convention and statepoint flags operand to STATEPOINT into two separate operands.

[pgavlin]

Summary

• Added documentation for the CC and Flags Locations created by statepoints.
• Added some suggested comments

[reames]

Minor comments below. Looks close to ready.

I'm mildly disturbed you didn't need to change anything else in the backend. Did you audit things like X86MCInstLowering and X86ISelLowering to make sure they weren't using raw indices into the operand list?

include/llvm/CodeGen/StackMaps.h

100 ↗

(On Diff #25371)

Why two enums? I suspect there's a semantic difference, possibly a name or comment is appropriate?

test/CodeGen/X86/statepoint-allocas.ll

75 ↗

(On Diff #25371)

With this change, you're changing the public format. Thus, we need to update: http://llvm.org/docs/Statepoints.html#stack-map-format

Having said that, it looks like the documentation never included the flags at all. Oops. Can you add entries for both calling convention and flags?

[pgavlin]

I did, yes.Nearly all of the code that would be sensitive to operand indices is in StackMap.{c,h}, and it is very good about using accessors and/or symbolic offsets.

include/llvm/CodeGen/StackMaps.h

100 ↗

(On Diff #25371)

There is a semantic difference:

• {NCallArgsPos, CallTargetPos} are absolute offsets from the start of the argument list
• {CCOfset, FlagsOffset, NumVMSArgsOffset} are relative offsets from the start of the meta-args

I'll add a comment to that effect.

test/CodeGen/X86/statepoint-allocas.ll

75 ↗

(On Diff #25371)

Will do.

[reames]

LGTM. One optional comment, no further review needed.

Thanks.

docs/Statepoints.rst

486 ↗

(On Diff #25596)

Can you add something to the effect of: This constant is a valid calling convention identifier (http://llvm.org/docs/LangRef.html#calling-conventions) for the version of LLVM used to generate the stackmap. There are no additional compatibility guarantees made for this field over what LLVM provides elsewhere w.r.t. these indentifiers.

llvm/trunk/docs/Statepoints.rst

 ================
 
 Locations for each pointer value which may need read and/or updated by
 the runtime or collector are provided via the :ref:`Stack Map format
 <stackmap-format>` specified in the PatchPoint documentation.
 
 Each statepoint generates the following Locations:
 
+* Constant which describes the calling convention of the call target. This
+  constant is a valid :ref:`calling convention identifier <callingconv>` for
+  the version of LLVM used to generate the stackmap. No additional compatibility
+  guarantees are made for this constant over what LLVM provides elsewhere w.r.t.
+  these identifiers.
+* Constant which describes the flags passed to the statepoint intrinsic
 * Constant which describes number of following deopt *Locations* (not
   operands)
 * Variable number of Locations, one for each deopt parameter listed in
   the IR statepoint (same number as described by previous Constant)
 * Variable number of Locations pairs, one pair for each unique pointer
   which needs relocated.  The first Location in each pair describes
   the base pointer for the object.  The second is the derived pointer
   actually being relocated.  It is guaranteed that the base pointer

llvm/trunk/include/llvm/CodeGen/StackMaps.h

public:
   /// \brief Get the next scratch register operand index.
   unsigned getNextScratchIdx(unsigned StartIdx = 0) const;
 };
 
 /// MI-level Statepoint operands
 ///
 /// Statepoint operands take the form:
 ///   <num call arguments>, <call target>, [call arguments],
-///   <StackMaps::ConstantOp>, <flags>,
+///   <StackMaps::ConstantOp>, <calling convention>,
+///   <StackMaps::ConstantOp>, <statepoint flags>,
 ///   <StackMaps::ConstantOp>, <num other args>, [other args],
 ///   [gc values]
 class StatepointOpers {
 private:
+  // These values are aboolute offsets into the operands of the statepoint
+  // instruction.
   enum {
     NCallArgsPos = 0,
     CallTargetPos = 1
   };
 
+  // These values are relative offests from the start of the statepoint meta
+  // arguments (i.e. the end of the call arguments).
+  enum {
+    CCOffset = 1,
+    FlagsOffset = 3,
+    NumVMSArgsOffset = 5
+  };
+
 public:
   explicit StatepointOpers(const MachineInstr *MI):
     MI(MI) { }
 
   /// Get starting index of non call related arguments
-  /// (statepoint flags, vm state and gc state).
+  /// (calling convention, statepoint flags, vm state and gc state).
   unsigned getVarIdx() const {
     return MI->getOperand(NCallArgsPos).getImm() + 2;
   }
 
   /// Returns the index of the operand containing the number of non-gc non-call
   /// arguments. 
   unsigned getNumVMSArgsIdx() const {
-    return getVarIdx() + 3;
+    return getVarIdx() + NumVMSArgsOffset;
   }
 
   /// Returns the number of non-gc non-call arguments attached to the
   /// statepoint.  Note that this is the number of arguments, not the number of
   /// operands required to represent those arguments.
   unsigned getNumVMSArgs() const {
     return MI->getOperand(getNumVMSArgsIdx()).getImm();
   }

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

 #define DEBUG_TYPE "statepoint-lowering"
 
 STATISTIC(NumSlotsAllocatedForStatepoints,
           "Number of stack slots allocated for statepoints");
 STATISTIC(NumOfStatepoints, "Number of statepoint nodes encountered");
 STATISTIC(StatepointMaxSlotsRequired,
           "Maximum number of stack slots required for a singe statepoint");
 
+static void pushStackMapConstant(SmallVectorImpl<SDValue>& Ops,
+                                 SelectionDAGBuilder &Builder, uint64_t Value) {
+  SDLoc L = Builder.getCurSDLoc();
+  Ops.push_back(Builder.DAG.getTargetConstant(StackMaps::ConstantOp, L,
+                                              MVT::i64));
+  Ops.push_back(Builder.DAG.getTargetConstant(Value, L, MVT::i64));
+}
+
 void StatepointLoweringState::startNewStatepoint(SelectionDAGBuilder &Builder) {
   // Consistency check
   assert(PendingGCRelocateCalls.empty() &&
          "Trying to visit statepoint before finished processing previous one");
   Locations.clear();
   RelocLocations.clear();
   NextSlotToAllocate = 0;
   // Need to resize this on each safepoint - we need the two to stay in
static void lowerIncomingStatepointValue(SDValue Incoming,
                                          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
-    Ops.push_back(Builder.DAG.getTargetConstant(StackMaps::ConstantOp,
-                                                Builder.getCurSDLoc(),
-                                                MVT::i64));
-    Ops.push_back(Builder.DAG.getTargetConstant(C->getSExtValue(),
-                                                Builder.getCurSDLoc(),
-                                                MVT::i64));
+    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 {
     // Otherwise, locate a spill slot and explicitly spill it so it
for (unsigned i = 0; i < Bases.size(); ++i) {
     const Value *Ptr = Ptrs[i];
     reservePreviousStackSlotForValue(Builder.getValue(Ptr), 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 = StatepointSite.getNumTotalVMSArgs();
-  Ops.push_back( Builder.DAG.getTargetConstant(StackMaps::ConstantOp,
-                                               Builder.getCurSDLoc(),
-                                               MVT::i64));
-  Ops.push_back(Builder.DAG.getTargetConstant(NumVMSArgs, Builder.getCurSDLoc(),
-                                              MVT::i64));
+  pushStackMapConstant(Ops, Builder, NumVMSArgs);
 
   assert(NumVMSArgs + 1 == std::distance(StatepointSite.vm_state_begin(),
                                          StatepointSite.vm_state_end()));
 
   // The vm state arguments are lowered in an opaque manner.  We do
   // not know what type of values are contained within.  We skip the
   // first one since that happens to be the total number we lowered
   // explicitly just above.  We could have left it in the loop and
#endif
   // Get position of register mask in the call
   SDNode::op_iterator RegMaskIt;
   if (CallHasIncomingGlue)
     RegMaskIt = CallNode->op_end() - 2;
   else
     RegMaskIt = CallNode->op_end() - 1;
   Ops.insert(Ops.end(), CallNode->op_begin() + 2, RegMaskIt);
 
-  // Add a leading constant argument with the Flags and the calling convention
-  // masked together
-  CallingConv::ID CallConv = CS.getCallingConv();
+  // Add a constant argument for the calling convention
+  pushStackMapConstant(Ops, *this, CS.getCallingConv());
+
+  // Add a constant argument for the flags
   uint64_t Flags = cast<ConstantInt>(CS.getArgument(2))->getZExtValue();
   assert(
       ((Flags & ~(uint64_t)StatepointFlags::MaskAll) == 0)
           && "unknown flag used");
-  const int Shift = 1;
-  static_assert(
-      ((~(uint64_t)0 << Shift) & (uint64_t)StatepointFlags::MaskAll) == 0,
-      "shift width too small");
-  Ops.push_back(DAG.getTargetConstant(StackMaps::ConstantOp, getCurSDLoc(),
-                                      MVT::i64));
-  Ops.push_back(DAG.getTargetConstant(Flags | ((unsigned)CallConv << Shift),
-                                      getCurSDLoc(), MVT::i64));
+  pushStackMapConstant(Ops, *this, Flags);
 
   // Insert all vmstate and gcstate arguments
   Ops.insert(Ops.end(), LoweredMetaArgs.begin(), LoweredMetaArgs.end());
 
   // Add register mask from call node
   Ops.push_back(*RegMaskIt);
 
   // Add chain

llvm/trunk/test/CodeGen/X86/statepoint-allocas.ll

 ; Large Constants
 ; Statepoint ID only
 ; CHECK: .quad  2882400000
 
 ; Callsites
 ; The GC one
 ; CHECK: .long  .Ltmp1-test
 ; CHECK: .short 0
-; CHECK: .short 3
+; CHECK: .short 4
+; 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+0]
 ; CHECK: .byte  2
 ; CHECK: .byte  8
 ; CHECK: .short 7
 ; CHECK: .long  0
 ; No Padding or LiveOuts
 ; CHECK: .short 0
 ; CHECK: .short 0
 ; CHECK: .align 8
 
 ; The Deopt one
 ; CHECK: .long  .Ltmp3-test2
 ; CHECK: .short 0
-; CHECK: .short 3
+; CHECK: .short 4
+; 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 (1)
 ; CHECK: .byte  4
 ; CHECK: .byte  8

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

 ; Large Constants
 ; Statepoint ID only
 ; CHECK: .quad  2882400000
 
 ; Callsites
 ; Constant arguments
 ; CHECK: .long  .Ltmp1-test
 ; CHECK: .short 0
-; CHECK: .short 10
+; 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

 ; Large Constants
 ; Statepoint ID only
 ; CHECK: .quad  2882400000
 
 ; Callsites
 ; Constant arguments
 ; CHECK: .long  .Ltmp3-test_derived_arg
 ; CHECK: .short 0
-; CHECK: .short 10
+; 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