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

[Philip Reames]

Much improved and almost there.  Lots of comments, one bug.

FYI, readability wise I had some trouble following the code.  Once you break it up, I might spot more things.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:310
@@ +309,3 @@
+      for (Instruction &I : BB)
+	if (isStatepoint(I))
+	  MadeChange |= SimplifyOffsetableRelocate(I);
----------------
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)

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

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:566
@@ +565,3 @@
+      AllRelocateCalls.push_back(U);
+  // We need atleast one base pointer relocation + one derived pointer
+  // relocation to mangle
----------------
I'd suggest using whitespace (blank lines) to break up the logic junks.  You might also want to split out a few static helpers.

================
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
----------------
Your comment here confuses me.  I think you mean that you'd have to insert the base relocate and then gep off that?

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

================
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
----------------
Shouldn't the first part of this be impossible and thus an assert?

================
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;
----------------
Indentation!  Are you using tabs?

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

================
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
----------------
I'm not sure doing this for *any* gep is a good idea.  I'd suggest starting with a small constant offsets only.  

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

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:626
@@ +625,3 @@
+	cast<Instruction>(Repl)->removeFromParent();
+	cast<Instruction>(Repl)->insertAfter(GEPBase);
+	Repl->takeName(Target);
----------------
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.

http://reviews.llvm.org/D6883

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