[PATCH] [GC] CodeGenPrep transform: simplify offsetable relocate

[Ramkumar Ramachandra]

Thanks for the detailed review! Expect the next iteration shortly.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:310
@@ +309,3 @@
+      for (Instruction &I : BB)
+	if (isStatepoint(I))
+	  MadeChange |= SimplifyOffsetableRelocate(I);
----------------
reames wrote:
> You've got a whitespace problem here.  Please fix indentation.
> 
> Also, you may be invalidating the iterator you're using here.  I'd strongly recommend collecting the statepoints into a SmallVector than processing them.  (i.e. two pass)
Whitespace: Nobody is maintaining the Emacs modes in-tree, so I'm using my own. I forgot about indent-tabs-mode.

SmallVector done. Why will the iterator get invalidated though? I'm not modifying the instruction in `SimplifyOffsetableRelocate`, am I?

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:313
@@ +312,3 @@
+
+    if (MadeChange)
+      ModifiedDT = true;
----------------
reames wrote:
> These changes should change the dom tree?  Why is this needed?
Removed.

I thought I needed this because the dominator tree is changing (when we reorder the base relocate to come before the derived relocate)?

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:590
@@ +589,3 @@
+    IntrinsicInst *Base = RelocateMap[BaseKey];
+    if (!Base)
+      // If we don't have a handle on the relocated base object, how can we gep
----------------
reames wrote:
> Your comment here confuses me.  I think you mean that you'd have to insert the base relocate and then gep off that?
That's a TODO then: spitting out a base relocate and gep'ing off that when there are enough derived relocate calls.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:596
@@ +595,3 @@
+      continue;
+    auto NewVal = RelocateMasterMap[Base];
+    RelocateMasterMap.erase(Base);
----------------
reames wrote:
> Can't this become:
> RelocateMasterMap[Base].push_back()
Um, yeah.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:610
@@ +609,3 @@
+      if (ThisRelocate.basePtrIndex() != MasterRelocate.basePtrIndex() ||
+	  ThisRelocate.basePtrIndex() == ThisRelocate.derivedPtrIndex()) {
+	// Not relocating a derived object with the original base object OR a
----------------
reames wrote:
> Shouldn't the first part of this be impossible and thus an assert?
Cruft left from my previous iterations. Converted into assert now.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:617
@@ +616,3 @@
+      auto Derived = dyn_cast<GetElementPtrInst>(ThisRelocate.derivedPtr());
+      if (Derived && Derived->getPointerOperand() == Base) {
+	SmallVector<Value *, 2> OffsetV;
----------------
reames wrote:
> reames wrote:
> > Indentation!  Are you using tabs?
> Early continue please, or if (auto Derived = ...) at least.
> 
> You need to break up this function for readability.  If you want specific suggestions, ask, but any reasonable division should do.  
Not sure what you mean: you want a `if (!Derived) continue;` so the user doesn't have to read till the end of the block to figure out that it's continuing?

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:620
@@ +619,3 @@
+	for (unsigned i = 1; i < Derived->getNumOperands(); i++)
+	  OffsetV.push_back(Derived->getOperand(i));
+	// Create a Builder and replace the target callsite with a gep
----------------
reames wrote:
> I'm not sure doing this for *any* gep is a good idea.  I'd suggest starting with a small constant offsets only.  
I've defined "small" as 20: so if there ConstantInt offsets, all less than 20, then gep.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:624
@@ +623,3 @@
+	Builder.SetCurrentDebugLocation(Target->getDebugLoc());
+	Value *Repl = Builder.CreateInBoundsGEP(GEPBase, makeArrayRef(OffsetV));
+	cast<Instruction>(Repl)->removeFromParent();
----------------
reames wrote:
> InBounds is wrong.  out of bounds geps for relocates are explicitly legal.
Okay, fixed.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:626
@@ +625,3 @@
+	cast<Instruction>(Repl)->removeFromParent();
+	cast<Instruction>(Repl)->insertAfter(GEPBase);
+	Repl->takeName(Target);
----------------
reames wrote:
> p.s.  By this point in the code, I've completely lost track of what Target and Repl refer to.  You need better variable names and to break this function up.
If my variable names are too long, I hit the 80-column limit; If they're too short, they aren't descriptive enough :|

I'll figure something out.

http://reviews.llvm.org/D6883

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/