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

Sanjoy Das sanjoy at playingwithpointers.com
Tue May 12 16:55:44 PDT 2015


================
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.
+
----------------
reames wrote:
> 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.  
Done.

================
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.
+
----------------
reames wrote:
> More detail is needed here.  What are the semantics of this patchable section?  Is it structured to be atomically patchable in any way?
Done.  I did not mention about the section begin atomically patchable since that seems somewhat tangential, but I've mentioned that there are no alignment guarantees.

================
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.
----------------
reames wrote:
> Merge w/previous paragraph.
Done.

================
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);
 
----------------
reames wrote:
> 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.
Done.

================
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");
----------------
reames wrote:
> 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.
Done.

http://reviews.llvm.org/D9546

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






More information about the llvm-commits mailing list