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

[Igor Laevsky]

REPOSITORY
  rL LLVM

================
Comment at: include/llvm/CodeGen/FunctionLoweringInfo.h:82
@@ +81,3 @@
+  // for consistency checks.
+  DenseMap<const Value*, std::pair<bool, int>> InvokeStatepointValueSlots;
+  
----------------
reames wrote:
> 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.
> 
> 
This should not be the problem anymore as I added statepoint as a key to the map.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:579
@@ +578,3 @@
+        // CopyFromReg from undefined register in visitGcRelocate.
+        Builder.ExportFromCurrentBlock(V);
+      }
----------------
sanjoy wrote:
> So is it correct to say that we need this because `gc_relocate` only "indirectly" uses `V`, in a manner invisible to LLVM?  For instance, if our representation of a `gc_relocate` was 
> 
> ```
> TOK = safepoint
> i8* V.relocated = gc_relocate(TOK, i8* V.original)
> ```
> 
> then we would not need this since `V.original` would get exported at its def if needed?
That's right, but only for values we are not spilling on the stack, like allocas. For spilled values even if we will export them we still need to know stack slot we spilled them to.

(I am not considering that we can not have gc_relocate using original value as it would mean use of unrelocated value after statepoint)

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:535
@@ +534,3 @@
+  if (StatepointSite.getCallSite().isInvoke()) {
+    for (GCRelocateOperands relocateOpers :
+           StatepointSite.getRelocates(StatepointSite)) {
----------------
reames wrote:
> 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.  
That is correct. We need to record in a StackMap and spill only unique SDValues. However we might visit two different gc_relocates relocating values with same SDValues. In order to handle this we are recording locations for all possible values. Simply for values with same SDValues we will have same locations.

================
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
----------------
reames wrote:
> 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
> }
Should not be the problem in updated code.

================
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
----------------
reames wrote:
> 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.  
Basic invariant here is that statepoint ranges are not intersecting. Or in other words - on entry to each statepoint all previous gc_relocates are already visited. 

This is not a new invariant, current code asserts it in  startNewStatepoint by checking "PendingGCRelocateCalls.empty()". Unfortunately I have not found easy way of asserting it for invoke statepoints, but IR verification should prevent such cases anyway.

http://reviews.llvm.org/D7798

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