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

[Philip Reames]

LGTM pending minor documentation and style comments.  No further review needed.


================
Comment at: docs/Statepoints.rst:318
@@ +317,3 @@
+The 'id' operand is a statepoint specific ID that is reported as the
+ID field in the generated stackmap.
+
----------------
I'd comment here that the meaning is up to the user.  It's an opaque value to LLVM.  You might also comment that it's not guaranteed to be unique due to code duplication.  

================
Comment at: docs/Statepoints.rst:321
@@ +320,3 @@
+If 'num patch bytes' is non-zero then the statepoint is lowered to a
+'num patch bytes' byte long nop sequence instead of a call.
+
----------------
More detail is needed here.  What are the semantics of this patchable section?  Is it structured to be atomically patchable in any way?

================
Comment at: docs/Statepoints.rst:329
@@ -321,1 +328,3 @@
 
+If 'num patch bytes' is non-zero then 'target' has to be the constant
+pointer null of the appropriate function type.
----------------
Merge w/previous paragraph.

================
Comment at: lib/Target/X86/X86MCInstLower.cpp:815
@@ -814,33 +814,3 @@
 
-  // Lower call target and choose correct opcode
-  const MachineOperand &CallTarget = StatepointOpers(&MI).getCallTarget();
-  MCOperand CallTargetMCOp;
-  unsigned CallOpcode;
-  switch (CallTarget.getType()) {
-  case MachineOperand::MO_GlobalAddress:
-  case MachineOperand::MO_ExternalSymbol:
-    CallTargetMCOp = MCIL.LowerSymbolOperand(
-        CallTarget, MCIL.GetSymbolFromOperand(CallTarget));
-    CallOpcode = X86::CALL64pcrel32;
-    // Currently, we only support relative addressing with statepoints.
-    // Otherwise, we'll need a scratch register to hold the target
-    // address.  You'll fail asserts during load & relocation if this
-    // symbol is to far away. (TODO: support non-relative addressing)
-    break;
-  case MachineOperand::MO_Immediate:
-    CallTargetMCOp = MCOperand::CreateImm(CallTarget.getImm());
-    CallOpcode = X86::CALL64pcrel32;
-    // Currently, we only support relative addressing with statepoints.
-    // Otherwise, we'll need a scratch register to hold the target
-    // immediate.  You'll fail asserts during load & relocation if this
-    // address is to far away. (TODO: support non-relative addressing)
-    break;
-  case MachineOperand::MO_Register:
-    CallTargetMCOp = MCOperand::CreateReg(CallTarget.getReg());
-    CallOpcode = X86::CALL64r;
-    break;
-  default:
-    llvm_unreachable("Unsupported operand type in statepoint call target");
-    break;
-  }
+  StatepointOpers SOpers(&MI);
 
----------------
Just to be clear, this is not trying to implement the shadow behaviour of a patchpoint right?  It's just raw patchable bytes?  Please clarify this in the documentation.

================
Comment at: lib/Transforms/Scalar/PlaceSafepoints.cpp:889
@@ -888,3 +888,3 @@
     CallInst *Call = Builder.CreateGCStatepointCall(
-        CS.getCalledValue(), makeArrayRef(CS.arg_begin(), CS.arg_end()), None,
-        None, "safepoint_token");
+        0, 0, CS.getCalledValue(), makeArrayRef(CS.arg_begin(), CS.arg_end()),
+        None, None, "safepoint_token");
----------------
I would mildly prefer you use the ID which was hard coded in the stackmap code before.  No reason to change behaviour if it's not needed.

http://reviews.llvm.org/D9546

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