[PATCH] Statepoint infrastructure for garbage collection

[Andrew Trick]

Sorry for the big delay, I don't generally get blocks of time large enough to review a patch this size. It's my fault for not insisting that you split it up into docs, IR representation, lowering to MI, and stackmap generation. Anyway, I can give a LGTM now as long you you address Juergen's coding convention comments and my comments inline and below.

Overall, I'm happy because we've agreed on a long-term plan for a generalized llvm.patchpoint to cover this use case and others. I just need to send out that proposal. In the meantime, this intrinsic lets you bootstrap the functionality.

It's somewhat shady that you create an IR instruction during SelectionDAG (CallInst::create). It's probably OK to use a temporary instruction like this (in fact we used to do it for patchpoint), but not ideal. I think we can live with it until a new uber-patchpoint comes along (no need to address it now).

Question: It looks like lowering may require statepoint and gc_relocate calls not to be interleaved? Is that true? If so, can you document that and ensure that it is verified somewhere?

The function getFrameIndexReferenceForGC() is not GC specific. Please use a more appropriate name, like getFrameIndexOffsetFromSP().

Please fix Sphinx warnings:

===================================
/s/patch/docs/Statepoints.rst:10: WARNING: Title underline too short.

Status/Warning
=======
/s/patch/docs/Statepoints.rst:60: WARNING: Title underline too short.

An Example Safepoint Sequence
========
/s/patch/docs/Statepoints.rst:60: WARNING: Title underline too short.

An Example Safepoint Sequence
========
/s/patch/docs/Statepoints.rst:158: WARNING: Title underline too short.

'''gc_relocate''' Intrinsic
^^^^^^^^^^^^^^^^^^^^^^
/s/patch/docs/Statepoints.rst:158: WARNING: Title underline too short.

'''gc_relocate''' Intrinsic
^^^^^^^^^^^^^^^^^^^^^^
/s/patch/docs/Statepoints.rst:234: WARNING: Title underline too short.

Polling for a Safepoint
============
/s/patch/docs/Statepoints.rst:234: WARNING: Title underline too short.

Polling for a Safepoint
============
looking for now-outdated files... none found
pickling environment... done
checking consistency... /s/patch/docs/Statepoints.rst:: WARNING: document isn't included in any toctree

================
Comment at: docs/Statepoints.rst:108
@@ +107,3 @@
+
+The 'unused' operand is unused and likely to be removed.  Please do not use.  
+
----------------
You should remove the "unused" operand, or commit to it and explain what it's for.

It's strange occurring between # call args and the variadic args. Shouldn't the pattern be something like:

flags, #args, args..., flags, #args, args...

================
Comment at: include/llvm/CodeGen/MachineInstr.h:91
@@ -90,3 +90,3 @@
 
-  uint8_t NumMemRefs;                   // Information on memory references.
+  uint8_t NumMemRefs;                  // Information on memory references.
   mmo_iterator MemRefs;
----------------
ributzka wrote:
> Whitespace
Accidental change.

================
Comment at: lib/IR/Verifier.cpp:1117-1125
@@ -1115,4 +1116,11 @@
     const User *U;
-    if (F.hasAddressTaken(&U))
-      Assert1(0, "Invalid user of intrinsic instruction!", U);
+    if (F.hasAddressTaken(&U)) {
+      bool ok = false;
+      if (const Instruction* I = dyn_cast<Instruction>(U)) {
+        if (isStatepoint(I)) {
+          ok = true;
+        }
+      }
+      Assert1(ok, "Invalid user of intrinsic instruction!", U);
+    }
   }
----------------
This is incomplete because it only verifies that one use of the address is a safepoint, not all users.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1136-1137
@@ +1135,4 @@
+    // Note: LLVM arranges the stack as:
+    // Args > Saved RetPC (<--FP) > CSRs > dynamic alignment (<--BP)
+    //      > "Stack Slots" (<--SP)
+    // We can always address StackSlots from RSP.  We can usually (unless
----------------
I think this comment is accidentally wrapped.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1185-1188
@@ +1184,6 @@
+
+  // Note: Adding the FI < 0 here you'd expect from the non-GC version of this
+  // function doesn't get through run-tests.sh --run-slow-tests.  Its not clear
+  // why not.  
+  //assert((-(Offset + StackSize)) % MFI->getObjectAlignment(FI) == 0);
+  return Offset + StackSize;
----------------
I don't understand this comment or why the assert is disabled.

http://reviews.llvm.org/D5683