[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
pagavlin at microsoft.com
Wed May 6 11:25:05 PDT 2015
Thanks for the feedback! I'll post a new version of the code with the requested changes later today.
================
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.
----------------
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?
================
Comment at: docs/Statepoints.rst:264
@@ +263,3 @@
+Values which need to be passed to GC transition code. They will be
+lowered normally, and will be made available as operands to the
+appropriate nodes in the selection DAG. It is assumed that these
----------------
reames wrote:
> What does "normally" mean?
I intended this to mean that transition arguments are not lowered in the same fashion as deopt args or GC args (e.g. no spilling is done). Instead, they are lowered however ISel would usually lower them.
================
Comment at: docs/Statepoints.rst:268
@@ +267,3 @@
+during) the execution of the callee. The '# transition args' field
+indicates how many oeprands are to be interpreted as 'transition
+parameters'.
----------------
reames wrote:
> oeprands -> operands
Yup. Thanks.
================
Comment at: docs/Statepoints.rst:271
@@ -260,2 +270,3 @@
+
The 'deopt parameters' arguments contain an arbitrary list of Values
which is meaningful to the runtime. The runtime may read any of these
----------------
reames wrote:
> Meta point: you never really define transition sequence or transition. Add section to the documentation with the background (preferably with an example) and reference it appropriately.
>
> As part of this, justify why the gc attributes isn't enough. Your function pointer case is fine, just mention it since the reader might be wondering.
Good point. I'll add the suggested background.
================
Comment at: docs/Statepoints.rst:485
@@ -473,3 +484,3 @@
gc "statepoint-example" {
- call i32 (void ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_isVoidf(void ()* @foo, i32 0, i32 0, i32 5, i32 0, i32 -1, i32 0, i32 0, i32 0)
+ call i32 (void ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_isVoidf(void ()* @foo, i32 0, i32 0, i32 0, i32 5, i32 0, i32 -1, i32 0, i32 0, i32 0)
ret i8 addrspace(1)* %obj
----------------
reames wrote:
> If you feel like it, you can delete the deopt args from this example. If not, I'll do it at some point.
I will leave the deopt args for now.
================
Comment at: include/llvm/IR/Statepoint.h:27
@@ -26,1 +26,3 @@
namespace llvm {
+namespace statepoint {
+/// The statepoint intrinsic accepts a set of flags as its third argument.
----------------
reames wrote:
> Consider using a class enum here. Possibly:
> enum class StatepointFlags {
> ....
> }
Will do.
================
Comment at: include/llvm/IR/Statepoint.h:76
@@ +75,3 @@
+ bool isGCTransition() {
+ return cast<ConstantInt>(StatepointCS.getArgument(2))->getZExtValue() &
+ statepoint::GCTransition;
----------------
reames wrote:
> I'd prefer to see a getFlags function added and this become, getFlags() & statepoint::GCTransition.
Will do.
================
Comment at: include/llvm/IR/Statepoint.h:120
@@ +119,3 @@
+ assert(Offset <= (int)StatepointCS.arg_size());
+ return StatepointCS.arg_begin() + Offset;
+ }
----------------
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.
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:600
@@ -600,1 +599,3 @@
// Call Node: Chain, Target, {Args}, RegMask, [Glue]
+ SDValue Chain = CallNode->getOperand(0);
+
----------------
sanjoy wrote:
> reames wrote:
> > You're changes here collide with a review Sanjoy has up for review. Please talk with him to separate the shared naming/refactoring pieces. Otherwise, whoever commits second will have a nasty merge conflict to fix.
> The latest version of my change (http://reviews.llvm.org/D9480) should not conflict with this one.
I don't mind dealing with any merge pain. It does look like Sanjoy is correct, however, and things should be pretty clean.
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:620
@@ +619,3 @@
+ I != E; ++I) {
+ Ops.push_back(getValue(*I));
+ if ((*I)->getType()->isPointerTy())
----------------
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?
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:675
@@ -640,3 +674,3 @@
// Add chain
Ops.push_back(CallNode->getOperand(0));
----------------
reames wrote:
> Doesn't this need to be Chain since you just added a new node?
Yes, good catch.
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:692
@@ +691,3 @@
+ if (ISP.isGCTransition()) {
+ SmallVector<SDValue, 8> Ops;
+
----------------
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.
================
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,
----------------
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?
================
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);
----------------
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.
================
Comment at: lib/IR/Verifier.cpp:1538
@@ -1534,3 +1537,3 @@
// Verify that the types of the call parameter arguments match
// the type of the wrapped callee.
----------------
reames wrote:
> Adding a check that says "if the callee has a gc attribute that is not the same as the caller, the transition bit should be set" would seem reasonable. Is there a reason not to do this? Or the inverse check for when the flag is set?
>
> Actually, such a check probably *doesn't* belong in the Verifier. You could have unreachable code that gets exposed during optimization and the resulting IR would fail that check, but still be semantically valid IR. It should be a lint rule and probably an instcombine rule to replace a mismatch with undef. :) A follow up patch or filed bug is fine.
I'll file a bug for this one :)
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17418
@@ +17417,3 @@
+ SelectionDAG &DAG) const {
+ // Replace with a NOOP for now.
+ SmallVector<SDValue, 2> Ops;
----------------
reames wrote:
> A bit of context would be good here. Readers of the code need to know why these are here.
Will add.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17423
@@ +17422,3 @@
+ if (Op->getGluedNode())
+ Ops.push_back(Op->getOperand(Op->getNumOperands() - 1));
+
----------------
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.
================
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);
+
----------------
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.
http://reviews.llvm.org/D9501
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list