[PATCH] Extend the statepoint intrinsic to allow statepoints to be marked as transitions from GC-aware code to code that is not GC-aware.

[Pat Gavlin]

================
Comment at: docs/Statepoints.rst:249
@@ -210,1 +248,3 @@
 
+Assuming that support for generating the necessary transition code for the
+"statepoint-example" GC strategy, the generated X86 assembly would be:
----------------
reames wrote:
> This paragraph gets confusing.  I think you'd be better off introducing a hypothetical GC, describe briefly how you might add support for such a GC, and then show the resulting code.  Just make it clear that such a GC is hypothetical.  :)
> 
> You might also mention where this is actually used.  (i.e. the CLR GC support.)  
> 
> I'd be tempted to have you guys actually add your strategy so that we can have the actual support conditional on the gc name and just use that for our example.  
Noted. I'll see if we can get a GC strategy for CoreCLR added in the very near future.

================
Comment at: include/llvm/IR/Statepoint.h:120
@@ +119,3 @@
+    assert(Offset <= (int)StatepointCS.arg_size());
+    return StatepointCS.arg_begin() + Offset;
+  }
----------------
reames wrote:
> pgavlin wrote:
> > reames wrote:
> > > Isn't this just return call_args_end()
> > > 
> > > It looks like you're defining the length argument to be part of the gc_transition_args range?  At first, I thought you were breaking with the convention, but then I noticed you copied this from vm_state_begin and friends.  That really doesn't seem like the right approach.  Having the length being consider part of the arg bundle just doesn't seem right.
> > > 
> > > I'm okay with this landing as is for the moment, but I'll likely fix all of these in the not too distant future.  
> > I thought that this pattern was a bit weird myself. The value I saw here was the assertion, but it's probably acceptable to write the assertion as
> > 
> > assert((call_args_end() - StatepointCS.args_begin()) <= (int)StatepointCS.arg_size());
> > 
> > However, I'd rather be consistent for now and fix all of these in a followup change.
> Sounds reasonable.  Any chance I can convince you to do the cleanup?  If not, I'll try to get to it myself.
Sure, I'm happy to do the cleanup.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:672
@@ +671,3 @@
+      (Flags & (uint64_t)StatepointFlags::MaskAll) == 0 && "unknown flag used");
+  const int Shift = 1;
+  static_assert(
----------------
reames wrote:
> This shift should be define in StackMaps.h for the StatepointOperands class with a more clear name.  
> 
> The other option is to just split this operand into 2.  I have no idea why I masked them to start with, that's just unnecessary complexity.  Might be better to do this as a separate change though.
I like the sound of the latter option. Do you mind if I do this in a separate change?

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:708
@@ +707,3 @@
+  // any of the operands is a pointer-typed, that operand is immediately
+  // followed by a SRCVALUE for the pointer that may be used during lowering
+  // (e.g. to form MachinePointerInfo values for loads/stores).
----------------
reames wrote:
> You could shorten this comment if desired by just referencing the format already defined for GC_TRANSITION_START.
Done.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:692
@@ +691,3 @@
+  if (ISP.isGCTransition()) {
+    SmallVector<SDValue, 8> Ops;
+
----------------
reames wrote:
> pgavlin wrote:
> > reames wrote:
> > > You've got name colision with the Ops at the outer scope.  Given the structure of the code, it most likely makes sense to either sink the statepoint operands into a scoped block of their own, or use another name here.  
> > The name hiding is deliberate, and IMHO more reliable: we shouldn't be accessing the operands to the STATEPOINT node itself here.
> I generally dislike any code where you can disable a declaration and silently get wrong behaviour.
> 
> Please change this.  I will not sign off on this with the name collision.  I don't really care how you resolve the collision, but I do care you resolve it.  
That's reasonable enough.

http://reviews.llvm.org/D9501

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