[PATCH] Statepoint infrastructure for garbage collection

Philip Reames listmail at philipreames.com
Wed Nov 5 17:28:52 PST 2014


Responding to review comments, updated change set coming soon.

================
Comment at: include/llvm/CodeGen/MachineInstr.h:91
@@ -90,3 +90,3 @@
 
-  uint8_t NumMemRefs;                   // Information on memory references.
+  uint16_t NumMemRefs;                  // Information on memory references.
   mmo_iterator MemRefs;
----------------
ributzka wrote:
> Are you really hitting the limitations of uint8_t for mem references? Changing this by one byte increases the overall size of a MachineInstr from 64 bytes to 72 bytes (due to padding).
We were at one point.  I'm not sure we still are.  I'm fine removing this in the change which initially gets upstreamed and revisiting this separately.  

================
Comment at: include/llvm/Target/Target.td:859
@@ +858,3 @@
+  let mayStore = 1;
+  let isNotDuplicable = 1;
+  let hasSideEffects = 1;
----------------
ributzka wrote:
> Is this a hard requirement for STATEPOINTs to work?
You know, I'm not sure.  I've forgotten why that was originally added.  I *think* it's probably legacy junk at this point.  

I'll remove it and see if I run into any failures.  

================
Comment at: include/llvm/Target/TargetFrameLowering.h:202-203
@@ -201,1 +201,4 @@
 
+  /// Same as above, except that the 'base register' will always be RSP, not
+  /// RBP on x86.  This should really be a parameterizable choice.
+  virtual int getFrameIndexReferenceForGC(const MachineFunction &MF, int FI,
----------------
ributzka wrote:
> Yup, couldn't agree more.
Is this a "fix before submit" comment?  Or just general agreement?  

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7330-7334
@@ +7329,7 @@
+  args.insert(args.end(), arg_begin, arg_end); 
+  CallInst* tmp = CallInst::Create(ActualCallee, args);
+  tmp->setTailCall(CI.isTailCall());
+  tmp->setCallingConv(CI.getCallingConv());
+  tmp->setAttributes(CI.getAttributes());
+  Builder.LowerCallTo(tmp, Builder.getValue(ActualCallee), false);
+
----------------
ributzka wrote:
> I think we had something similar like this before. I think we changed it because we didn't wan't to modify the LLVM IR by creating a new function call at this point.
I don't much care for this either, but at this point, it's fairly well tested and works.  I'm fine cleaning this up, but do you mind if we land this first then work incrementally?

http://reviews.llvm.org/D5683






More information about the llvm-commits mailing list