[PATCH] Statepoint infrastructure for garbage collection

[Philip Reames]

On 10/09/2014 03:39 PM, Robin Morisset wrote:
> I don't know enough about this topic to comment on the details of the ideas (although they seem reasonable and match what had been discussed on the mailing list), but I have a read part of the patch and found a few typos and have a few inline comments.
Thanks for the comments.  I'll mostly make the changes you suggest. 
Comments inline for a few.
>
> Could you also include more contexts in your patches (-U999999 if you produce your patches by svn/git diff)? It generally makes it easier to review changes on Phabricator.
> Thanks for working on this.
Will do.
>
> ================
> Comment at: include/llvm/CodeGen/MachineInstr.h:91
> @@ -90,2 +90,3 @@
>   
> -  uint8_t NumMemRefs;                   // Information on memory references.
> +  // BEGIN CHANGE: NumMemRefs was a uint8_t, which is too
> +  // small to hold the number of gc ptrs in some statepoints.
> ----------------
> What are these 'BEGIN CHANGE/END CHANGE'  comments ?
This will be removed.  I thought I'd gotten all of them.  This was 
helping me with merge conflicts in our branch.
>
> ================
> Comment at: include/llvm/IR/Intrinsics.td:500
> @@ +499,3 @@
> +                                llvm_vararg_ty],
> +                               [Throws]>;
> +
> ----------------
> Is this necessary ? I don't see in the documentation any case where the statepoint can throw an exception. And if the goal is just to say it can have side-effects on memory, that is the default if there is no annotation according to Intrinsics.td
Right now, there's no purpose to this.  We'll be supporting invokable 
statepoints eventually though.  I can remove it for now if that's the 
preference.
>
> ================
> Comment at: include/llvm/IR/Intrinsics.td:502
> @@ +501,3 @@
> +
> +def int_gc_result_int : Intrinsic<[llvm_anyint_ty], [llvm_i32_ty]>;
> +def int_gc_result_float : Intrinsic<[llvm_anyfloat_ty], [llvm_i32_ty]>;
> ----------------
> These (and gc_relocate) should probably have the attribute IntrNoMem if I understood correctly your documentation.
>
> ================
> Comment at: lib/CodeGen/StackMaps.cpp:285
> @@ +284,3 @@
> +  StatepointOpers opers(&MI);
> +  // Record all the deopt and gc operands (they're contigious and run from the
> +  // initial index to the end of the operand list)
> ----------------
> contigious -> contiguous
>
> ================
> Comment at: lib/IR/LLVMContext.cpp:252
> @@ -251,1 +251,2 @@
>   }
> +
> ----------------
> Unnecessary whitespace change.
>
> ================
> Comment at: lib/IR/Statepoint.cpp:15
> @@ +14,3 @@
> +  const Function *F = CS.getCalledFunction();
> +  if (F && F->getIntrinsicID() == Intrinsic::statepoint) {
> +    return true;
> ----------------
> Could be simplified in "return F && F->getIntrinsicID() == Intrinsic::statepoint"
>
> ================
> Comment at: lib/IR/Statepoint.cpp:32
> @@ +31,3 @@
> +bool llvm::isGCRelocate(const ImmutableCallSite &CS) {
> +  if (CS.isCall()) {
> +    return isGCRelocate(CS.getInstruction());
> ----------------
> Could be simplified in "return CS.isCall() && isGCRelocate(CS.getInstruction())"
>
> ================
> Comment at: lib/IR/Statepoint.cpp:47
> @@ +46,3 @@
> +bool llvm::isGCResult(const ImmutableCallSite &CS) {
> +  if (CS.isCall()) {
> +    return isGCResult(CS.getInstruction());
> ----------------
> Could be simplified in "return CS.isCall() && isGCRelsult(CS.getInstruction())"
>
> ================
> Comment at: lib/IR/Statepoint.cpp:96
> @@ +95,3 @@
> +    // this is specific to our runtime and is not generally applicable.
> +    return false;
> +  }
> ----------------
> This should probably be changed before upstreaming if it depends on your runtime.
>
> ================
> Comment at: lib/IR/Verifier.cpp:1120
> @@ +1119,3 @@
> +      if (const Instruction* I = dyn_cast<Instruction>(U)) {
> +        if (isStatepoint( const_cast<Instruction*>(I))) {
> +          ok = true;
> ----------------
> It is not clear to me why the const_cast is required: I is a const Instruction *, and isStatepoint accepts a const Instruction *.
>
> ================
> Comment at: lib/IR/Verifier.cpp:2561
> @@ +2560,3 @@
> +               gcRelocFn->getIntrinsicID() == Intrinsic::gc_relocate),
> +              "gc.result or gc.result are the only value uses of statepoint", &CI);
> +      if (gcRelocFn->getIntrinsicID() == Intrinsic::gc_result_int ||
> ----------------
> gc.result -> gc.relocate
>
> ================
> Comment at: lib/Target/X86/X86FrameLowering.h:74
> @@ +73,3 @@
> +  int getFrameIndexOffsetForGC(const MachineFunction &MF, int FI) const;
> +  /*override*/
> +  int getFrameIndexReferenceForGC(const MachineFunction &MF, int FI,
> ----------------
> Why not just use the override keyword ?
Old code from before the c++11 switch.  It needs updated.
>
> ================
> Comment at: lib/Target/X86/X86ISelLowering.cpp:20621
> @@ +20620,3 @@
> +    // As an implementation detail, STATEPOINT shares the STACKMAP format at
> +    // this point in the process.  We devirge later
> +    return emitPatchPoint(MI, BB);
> ----------------
> devirge -> diverge
>
> ================
> Comment at: lib/Target/X86/X86MCInstLower.cpp:861
> @@ +860,3 @@
> +
> +    // This is likelly to be patchable call, so we need to ensure that it will
> +    // not cross 16-byte boundary as VM requires from us.
> ----------------
> likelly -> likely
>
> ================
> Comment at: lib/Target/X86/X86MCInstLower.cpp:862
> @@ +861,3 @@
> +    // This is likelly to be patchable call, so we need to ensure that it will
> +    // not cross 16-byte boundary as VM requires from us.
> +    // 8-byte alignment is enough because this instruction is 5-byte long and
> ----------------
> Is this invariant something general to LLVM and documented as such, or specific to your application (I am asking because the comment talks about a 'VM')?
Hm, this is specific to us.   I'm tempted to just add the patchability 
requirements into the docs, but honestly, that part isn't fully baked 
yet.  I don't want to lock it down just yet. Suggestions on how to 
handle this?
>
> ================
> Comment at: lib/Target/X86/X86MCInstLower.cpp:870
> @@ +869,3 @@
> +    call_target_mcop = MCOperand::CreateImm(call_target.getImm());
> +    // It is legal to use this opcode because vm guarantees us that all call
> +    // addresses would be in 32bit range.
> ----------------
> Same question: where do this guarantee come from
>
> ================
> Comment at: test/Verifier/statepoint-non-gc-ptr.ll:1
> @@ +1,2 @@
> +; RUN: not opt -S %s 2>&1 | FileCheck %s
> +; CHECK: relocating non gc
> ----------------
> It is not clear exactly what this tests
It's testing the verifiers ability to report on invalid IR, but I'll 
clarify.
>
> http://reviews.llvm.org/D5683
>
>