[PATCH] Statepoint infrastructure for garbage collection
Robin Morisset
morisset at google.com
Thu Oct 9 15:39:02 PDT 2014
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
More information about the llvm-commits
mailing list