[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
Tue May 5 17:29:00 PDT 2015
General approach seems reasonable. The documentation needs major improvement though before this is ready to go in. If we hadn't talked about this offline, I would have been very confused about what this patch was trying to accomplish.
================
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.
----------------
Which bit of the flags is used for this? From the current documentation, it's not clear how to use these.
================
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
----------------
What does "normally" mean?
================
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'.
----------------
oeprands -> operands
================
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
----------------
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.
================
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
----------------
If you feel like it, you can delete the deopt args from this example. If not, I'll do it at some point.
================
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.
----------------
Consider using a class enum here. Possibly:
enum class StatepointFlags {
....
}
================
Comment at: include/llvm/IR/Statepoint.h:76
@@ +75,3 @@
+ bool isGCTransition() {
+ return cast<ConstantInt>(StatepointCS.getArgument(2))->getZExtValue() &
+ statepoint::GCTransition;
----------------
I'd prefer to see a getFlags function added and this become, getFlags() & statepoint::GCTransition.
================
Comment at: include/llvm/IR/Statepoint.h:120
@@ +119,3 @@
+ assert(Offset <= (int)StatepointCS.arg_size());
+ return StatepointCS.arg_begin() + Offset;
+ }
----------------
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.
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:600
@@ -600,1 +599,3 @@
// Call Node: Chain, Target, {Args}, RegMask, [Glue]
+ SDValue Chain = CallNode->getOperand(0);
+
----------------
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.
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:620
@@ +619,3 @@
+ I != E; ++I) {
+ Ops.push_back(getValue(*I));
+ if ((*I)->getType()->isPointerTy())
----------------
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.
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:675
@@ -640,3 +674,3 @@
// Add chain
Ops.push_back(CallNode->getOperand(0));
----------------
Doesn't this need to be Chain since you just added a new node?
================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:692
@@ +691,3 @@
+ if (ISP.isGCTransition()) {
+ SmallVector<SDValue, 8> Ops;
+
----------------
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.
================
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,
----------------
By converting to a 32 bit value, I believe you have silent truncation here. This should probably be a uint64_t.
================
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);
----------------
A more extensible way to express this would to add a MaskAllSet to the enum, then write this as:
Flags & ~MaskAllSet == 0
================
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.
----------------
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.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17418
@@ +17417,3 @@
+ SelectionDAG &DAG) const {
+ // Replace with a NOOP for now.
+ SmallVector<SDValue, 2> Ops;
----------------
A bit of context would be good here. Readers of the code need to know why these are here.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:17423
@@ +17422,3 @@
+ if (Op->getGluedNode())
+ Ops.push_back(Op->getOperand(Op->getNumOperands() - 1));
+
----------------
Just use getGluedNode?
================
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);
+
----------------
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?
http://reviews.llvm.org/D9501
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list