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

[Philip Reames]

Please make the changes suggested in previous rounds of discussion.  I do not plan to respond further until at least the minor items are addressed.  If you *disagree* with a proposed change, we can discuss, but anything which can be addressed without discussion should be done before replying.  Reviewing a concrete piece of code is much easier than keeping track of proposed revisions.


REPOSITORY
  rL LLVM

================
Comment at: include/llvm/CodeGen/FunctionLoweringInfo.h:82
@@ +81,3 @@
+  // for consistency checks.
+  DenseMap<const Value*, std::pair<bool, int>> InvokeStatepointValueSlots;
+  
----------------
igor-laevsky wrote:
> reames wrote:
> > 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;
> You are right, we are not guaranteed that same stack slot is allocated for the same value in different statepoints. But we are guaranteed that when we start processing new safepoint with same value we already finished processing all gc relocates of the previous one and can safely discard previously saved location. 
> 
> See comment on line 557 in StatepointLowering.cpp for more info. Test case "test_same_val" checks this case.
I'm having trouble understanding your comment in context of the comment in the code.  Are you saying we remove entries from this map after visiting a particular safepoint?  That doesn't seem to agree with the comment in code.



================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:535
@@ +534,3 @@
+  if (StatepointSite.getCallSite().isInvoke()) {
+    for (GCRelocateOperands relocateOpers :
+           StatepointSite.getRelocates(StatepointSite)) {
----------------
igor-laevsky wrote:
> reames wrote:
> > 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.
> In Ptr's array we only have values with unique SDValues. But we need to store locations of the all possible relocated values. Notice that this loop is over StatepointSite.getRelocates().
> 
Er, yes.  But by definition, the unique SDValues *are* all the values that need to be relocated.  That was my point.  

================
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
----------------
igor-laevsky wrote:
> reames wrote:
> > Er, I don't think there should be such a case.  Do you have an example or should this be an assert?
> There is such cases for constants, allocas and undefs. See "test_null_undef" and "test_alloca_and_const" tests. 
Please clarify the comment to include the first half of your response.  

Also, the fact this is the *exact* same case as the previous but for the invoke handling seems questionable.  I might suggest:
if (isCall) {
  above code;
} else {
  new code with comment
}

================
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
----------------
igor-laevsky wrote:
> reames wrote:
> > 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?
> At the beginning of each statepoint we know that all previous relocates were visited (otherwise statepoint intervals will intersect and it does not have any sense). It means that we are not going to need locations for this relocates and can reuse all allocated slots.
> 
> To be clear, by "previous relocates" I mean relocates that dominate current safepoint. For example relocates on exceptional path could have been left unvisited. This is not problematic because we are still reusing slots only at the point dominated by only visited relocates. I.e when we start reusing slots on exceptional path all exceptional relocates should be visited.
I'm still confused here.  Really, really basic question: What is the invariant this code is trying to uphold?  Is the same Value always spilled to the same slot?  Please summarize this in a block comment in the revised code and point me to it.

http://reviews.llvm.org/D7798

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