[PATCH] Lower invoke statepoints and gc.results tied to them.

Philip Reames listmail at philipreames.com
Thu Feb 19 14:38:12 PST 2015


Comments inline.

You need tests for this.

Also, your code appears to have handling for gc.result even though your description says that's not handled.  Which is it?


REPOSITORY
  rL LLVM

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1048
@@ +1047,3 @@
+  SDValue copyFromReg = getCopyFromRegs(V, V->getType());
+  if (copyFromReg.getNode()) {
+    return copyFromReg;
----------------
I'm a bit uncomfortable signing off on this part of the change since I'm unfamiliar with the details of the codepath.  I'll ask that you wait for a signoff from someone more familiar with SelectionDAG as a whole.  

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:257
@@ +256,3 @@
+      // so we need to export it now.
+      unsigned reg = Builder.FuncInfo.CreateRegs(Tmp->getType());
+      Builder.CopyValueToVirtualRegister(Tmp, reg);
----------------
I'm still confused about why we need this.  Why doesn't the standard export mechanism work here?  Simply producing the value and letting it be exported normally seems like it would work?

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:297
@@ -277,2 +296,3 @@
   int Sanity = 0;
   while (CallEnd->getOpcode() != ISD::CALLSEQ_END) {
+    assert(CallEnd->getNumOperands() >= 1 &&
----------------
Just to be clear, the reason for this is that CallEnd might start with the eh_label which doesn't have a glue node?  

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:669
@@ +668,3 @@
+    // and getValue() will use CopyFromReg of the wrong type (always i32 in our case)
+    PointerType *calleeType = cast<PointerType>(
+                                ImmutableStatepoint(I).actualCallee()->getType());
----------------
Doesn't this depend on what SDValue we set for a specific instruction?   As above, I'm not convinced this is necessary.

Also, CamelCase naming per llvm docs.

http://reviews.llvm.org/D7760

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






More information about the llvm-commits mailing list