[PATCH] Statepoint infrastructure for garbage collection

[Philip Reames]

On 11/21/2014 05:24 PM, Andrew Trick wrote:
> 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.
Thanks.  I'm out of office this week, but I'll start submitting pieces 
on Monday.  I plan to submit the changes in sections.  For each 
submission, I your comments and run them through clang-format. I'll 
likely separate each submission by a day or so to let the bots cycle clean.

I'm not sure splitting the changes here would have actually helped. 
Reviewing the changes without the whole picture would have been hard.  
The thought of the effort involve in keeping distinct patches in sync 
with both upstream and my internal branch over the time this has taken 
just makes me shutter.
>
> 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.
I see the long term plan as being just that: a long term plan.  I expect 
that statepoints will persist for some time.
>
> 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).
I agree; this is a bit ugly.  I'm open to revising this if you have 
concrete suggestions.  I'm happy to prepare a separate patch for review.
>
> 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?
Hm, good question.  Yes, this is an unspoken assumption.  It will be 
caught via an assertion, but the reporting could definitely be better.  
I'll add a TODO for now and prepare a separate patch.
>
> 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
>
>