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

Philip Reames listmail at philipreames.com
Wed May 6 16:18:37 PDT 2015


Responding to questions in comments, not yet actually reviewing most recent code.


================
Comment at: docs/Statepoints.rst:253
@@ -252,1 +252,3 @@
+statepoint. This is currently only used to mark certain statepoints
+as transitions from GC-aware code to code that is not GC-aware.
 
----------------
pgavlin wrote:
> reames wrote:
> > Which bit of the flags is used for this?  From the current documentation, it's not clear how to use these.  
> The first bit. Should I add a table for the flags bits here, or out-of-line?
No strong preference.  Do what you think creates more readable docs.  

================
Comment at: include/llvm/IR/Statepoint.h:120
@@ +119,3 @@
+    assert(Offset <= (int)StatepointCS.arg_size());
+    return StatepointCS.arg_begin() + Offset;
+  }
----------------
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.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:620
@@ +619,3 @@
+         I != E; ++I) {
+      Ops.push_back(getValue(*I));
+      if ((*I)->getType()->isPointerTy())
----------------
pgavlin wrote:
> reames wrote:
> > What's the format here?  Are what are the source values used for?  This should probably be documented somewhere as well.  At least in comment form, probably as actual documentation.  
> The SrcValues are used so that later lowering can form MachinePointerInfos. As suggested, I'll document the format. Should I just add a section to the existing statepoint docs?
Yep.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:692
@@ +691,3 @@
+  if (ISP.isGCTransition()) {
+    SmallVector<SDValue, 8> Ops;
+
----------------
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.  

================
Comment at: lib/IR/Verifier.cpp:1534
@@ +1533,3 @@
+         "gc.statepoint flags must be constant integer", &CI);
+  const int Flags = cast<ConstantInt>(FlagsV)->getZExtValue();
+  Assert(Flags == statepoint::None || Flags == statepoint::GCTransition,
----------------
pgavlin wrote:
> reames wrote:
> > By converting to a 32 bit value, I believe you have silent truncation here.  This should probably be a uint64_t.
> Yes, you're correct. This pattern is common throughout statepoint validation--should I fix the other instances now, or file a bug and follow up with another patch?
File a bug, fix later.  I can't ask you to fix all of my bugs in one go.  :)

================
Comment at: lib/IR/Verifier.cpp:1535
@@ +1534,3 @@
+  const int Flags = cast<ConstantInt>(FlagsV)->getZExtValue();
+  Assert(Flags == statepoint::None || Flags == statepoint::GCTransition,
+         "unknown flag used in gc.statepoint flags argument", &CI);
----------------
pgavlin wrote:
> reames wrote:
> > A more extensible way to express this would to add a MaskAllSet to the enum, then write this as:
> > Flags & ~MaskAllSet == 0
> I agree. I had originally intended to do just that, but could not think of a terribly robust way of defining MaskAllSet.
In the enum declaration: 
MaskAllSet = GCTransition | NextFlag | NextFlag,

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17423
@@ +17422,3 @@
+  if (Op->getGluedNode())
+    Ops.push_back(Op->getOperand(Op->getNumOperands() - 1));
+
----------------
pgavlin wrote:
> reames wrote:
> > Just use getGluedNode?
> Using getGluedNode would change the code to something like:
> 
> SDNode *GluedNode = Op->getGluedNode();
> if (GluedNode)
>   Ops.push_back(SDValue(GluedNode, GluedNode->getNumValues() - 1);
> 
> I don't feel terribly strongly one way or the other: either way we're following a convention (glue is the terminal {operand,value}). FWIW, the code that is already present is a pretty common pattern.
Eh, I don't really care here.  I'll defer to your preference.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17427
@@ +17426,3 @@
+  SDVTList VTs = DAG.getVTList(MVT::Other, MVT::Glue);
+  SDValue NOOP(DAG.getMachineNode(X86::NOOP, SDLoc(Op), VTs, Ops), 0);
+
----------------
pgavlin wrote:
> reames wrote:
> > This looks slightly suspicious to me.  I wouldn't necessarily expect a noop node to take a control and glue.  Are you sure the rest of the backend will expect this?
> I tested this by running all of the statepoint lowering tests with the GC transition flag set to '1'. There were no failures (or even asm diffs, as the NOOP instructions are removed by dead machine instruction elimination), which seems like a reasonable indication that the backend can tolerate this.
Agreed.

http://reviews.llvm.org/D9501

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






More information about the llvm-commits mailing list