[PATCH] [Statepoints] Support for "patchable" statepoints.

[Sanjoy Das]

================
Comment at: docs/Statepoints.rst:145
@@ -144,3 +144,3 @@
          gc "statepoint-example" {
-    %0 = call i32 (void ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_isVoidf(void ()* @foo, i32 0, i32 0, i32 0, i8 addrspace(1)* %obj)
+    %0 = call i32 (void ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_isVoidfi64 0, i32 0, (void ()* @foo, i32 0, i32 0, i32 0, i8 addrspace(1)* %obj)
     %obj.relocated = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(i32 %0, i32 4, i32 4)
----------------
pgavlin wrote:
> Typo:
> 
> `@llvm.experimental.gc.statepoint.p0f_isVoidfi64 0, i32 0, (void ()* @foo, i32 0`
> 
> should be:
> 
> `@llvm.experimental.gc.statepoint.p0f_isVoidf(void ()* @foo, i64 0, i32 0, i32 0`
Fixed.

================
Comment at: include/llvm/IR/Intrinsics.td:532
@@ -532,1 +531,3 @@
+                               [llvm_i64_ty, llvm_i32_ty,
+			        llvm_anyptr_ty, llvm_i32_ty,
                                 llvm_i32_ty, llvm_vararg_ty]>;
----------------
pgavlin wrote:
> Formatting is off here.
Fixed.  Somehow my editor inserted tabs here and I could not tell the difference visually.

================
Comment at: include/llvm/IR/Statepoint.h:92
@@ +91,3 @@
+
+  /// Return the ID associated wit this statepoint.
+  uint64_t getID() {
----------------
pgavlin wrote:
> s/wit/with/
Fixed.

================
Comment at: include/llvm/IR/Statepoint.h:101
@@ -86,1 +100,3 @@
+    const Value *NumPatchBytesVal = StatepointCS.getArgument(NumPatchBytesPos);
+    return cast<ConstantInt>(NumPatchBytesVal)->getZExtValue();
   }
----------------
pgavlin wrote:
> An assertion that `NumPatchBytesVal` fits in 32 bits might be nice here.
Fixed.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:591-600
@@ -590,6 +590,12 @@
 
   // TODO: Currently, all of these operands are being marked as read/write in
   // PrologEpilougeInserter.cpp, we should special case the VMState arguments
   // and flags to be read-only.
   SmallVector<SDValue, 40> Ops;
 
+  // Add the <id> and <numBytes> constants.
+
+  Ops.push_back(DAG.getTargetConstant(ISP.getID(), getCurSDLoc(), MVT::i64));
+  Ops.push_back(
+      DAG.getTargetConstant(ISP.getNumPatchBytes(), getCurSDLoc(), MVT::i32));
+
----------------
pgavlin wrote:
> It might be nice to move the definition of `Ops` and the subsequent pushes closer to their use (i.e. somewhere near line 650).
Done.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:602
@@ -596,1 +601,3 @@
+
+  // Calculate and push starting position of vmstate arguments
   // Call Node: Chain, Target, {Args}, RegMask, [Glue]
----------------
pgavlin wrote:
> Comment is stale.
Fixed.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:673
@@ -665,3 +672,3 @@
   CallingConv::ID CallConv = CS.getCallingConv();
-  uint64_t Flags = cast<ConstantInt>(CS.getArgument(2))->getZExtValue();
+  uint64_t Flags = cast<ConstantInt>(CS.getArgument(4))->getZExtValue();
   assert(
----------------
pgavlin wrote:
> s/4/ImmutableStatepoint::FlagsPos/
Changed to `ISP.getFlags()`.

================
Comment at: lib/IR/Verifier.cpp:1510-1511
@@ +1509,4 @@
+         &CI);
+  const uint32_t NumPatchBytes =
+      cast<ConstantInt>(NumPatchBytesV)->getZExtValue();
+  Assert(NumPatchBytes >= 0, "gc.statepoint number of patchable bytes must be "
----------------
pgavlin wrote:
> Might be nice to have a check that `NumPatchBytes` fits in 32 bits. This condition should be satisfied by the intrinsic signature, but a little paranoia isn't necessarily bad. No strong feeling here.
Agreed.  I added an `assert` (not an `Assert`) for this.

================
Comment at: lib/IR/Verifier.cpp:1552
@@ -1531,3 +1551,3 @@
 
-  const Value *FlagsV = CS.getArgument(2);
+  const Value *FlagsV = CS.getArgument(4);
   Assert(isa<ConstantInt>(FlagsV),
----------------
pgavlin wrote:
> No need to fix this now, necessarily, but it would be nice to replace these literals with the relevant constants from Statepoint.h.
Agreed; good idea, but should probably happen in a separate patch.

http://reviews.llvm.org/D9546

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/