[PATCH] Statepoint infrastructure for garbage collection

[Robin Morisset]

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.

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.

================
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 ?

================
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

================
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 ?

================
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')?

================
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

http://reviews.llvm.org/D5683