[PATCH] Support of the gc.relocates for invoke statepoints

[Philip Reames]

Is there a missing part to this patch?  The current code doesn't seem complete/correct.


REPOSITORY
  rL LLVM

================
Comment at: include/llvm/CodeGen/FunctionLoweringInfo.h:82
@@ +81,3 @@
+  // for consistency checks.
+  DenseMap<const Value*, std::pair<bool, int>> InvokeStatepointValueSlots;
+  
----------------
I don't think we're actually guaranteed that the same stack slot is used across all statepoints.  Are we?  If not, this needs to be keyed by a pair of derived pointer and statepoint instruction.

Consider:
V = ...
if (X) statepoint; else statepoint;

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:533
@@ +532,3 @@
+
+  // For invoke statepoint we need to export locations of spilled values.
+  if (StatepointSite.getCallSite().isInvoke()) {
----------------
It's not all the spilled values here, just the relocated ones.  Can you fix the comment?

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:535
@@ +534,3 @@
+  if (StatepointSite.getCallSite().isInvoke()) {
+    for (GCRelocateOperands relocateOpers :
+           StatepointSite.getRelocates(StatepointSite)) {
----------------
Given we already have the derived pointers above, can't this be rewritten as a loop over Ptrs?

Actually, I would have expected to see this as part of the previous loop.  If lowerIncomingStatepointValue returned the lowered value, this code could be expressed neatly within the previously loop.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:542
@@ +541,3 @@
+      if (loc.getNode()) {
+        Builder.FuncInfo.InvokeStatepointValueSlots[V] =
+          std::make_pair(true, cast<FrameIndexSDNode>(loc)->getIndex());
----------------
See my comment above about coliding stack slots.  If this isn't possible, please add an assertion.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:681
@@ +680,3 @@
+  // purposes for them.
+  if (CS.isInvoke())
+    StatepointLowering.clearValidationInfo();
----------------
It seems like it would be easier to just not run the debug loop above for invokes.  Am I missing something?


================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:722
@@ -685,1 +721,3 @@
 void SelectionDAGBuilder::visitGCRelocate(const CallInst &CI) {
+  GCRelocateOperands relocateOpers(&CI);
+
----------------
CamelCase - er wait, you're just moving this.  Remind me to rename this as soon as your patch lands.  You don't need to and shouldn't change the name here.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:746
@@ +745,3 @@
+  if ((relocateOpers.isTiedToInvoke() &&
+       !FuncInfo.InvokeStatepointValueSlots[DerivedPtr].first)) {
+    // In this case we have visited invoke statepoint value
----------------
Er, I don't think there should be such a case.  Do you have an example or should this be an assert?

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:755
@@ -701,2 +754,3 @@
+
   SDValue Loc = StatepointLowering.getRelocLocation(SD);
   // Emit new load if we did not emit it before
----------------
This really doesn't look like right.  The allocation logic used inside spilling is assuming it can reuse slots.  It looks like you've essentially created an alternate mechanism for recording locations in invokes, but not updated the stack slot reuse/allocation code paths.  Am I missing something here?

http://reviews.llvm.org/D7798

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