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

[Philip Reames]

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:310
@@ +309,3 @@
+      for (Instruction &I : BB)
+	if (isStatepoint(I))
+          Statepoints.push_back(&I);
----------------
Whitespace again.

Do us both a favor and run this diff through clang-format.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:541
@@ +540,3 @@
+// derived pointer relocation instructions given a vector of all relocate calls
+static void ComputeBaseDerivedRelocateMap(const SmallVector<User *, 2>
+                                          &AllRelocateCalls,
----------------
llvm style is computeBase...

Make the param SmallVectorImpl so that it's size independent.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:553
@@ +552,3 @@
+    GCRelocateOperands ThisRelocate(U);
+    IntrinsicInst *I = dyn_cast<IntrinsicInst>(U);
+    auto K = std::make_pair(ThisRelocate.basePtrIndex(),
----------------
This should be a cast since it's never expected to fail.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:594
@@ +593,3 @@
+// computes a replacement, and affects it.
+static void AffectRelocateReplacements(IntrinsicInst *GEPBase,
+                                       const SmallVector<IntrinsicInst *, 2>
----------------
I think GEPBase might be more clearly named "RelocatedBase".

Also, "affect" is a slightly odd verb.  Possibly: SimplifyOneRelocate?

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:596
@@ +595,3 @@
+                                       const SmallVector<IntrinsicInst *, 2>
+                                       &Targets, bool &MadeChange)
+{
----------------
Please make MadeChange a normal return value rather than return by reference.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:614
@@ +613,3 @@
+
+    if (Derived->getPointerOperand() == Base) {
+      SmallVector<Value *, 2> OffsetV;
----------------
I might write this as:
if (!Derived || Derived->getPointerOperand() != Base)
  continue;

But this is minor.  I'll defer to your preference.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:616
@@ +615,3 @@
+      SmallVector<Value *, 2> OffsetV;
+      if (!GetGEPSmallConstantIntOffsetV(Derived, OffsetV))
+        return;
----------------
To more clearly seperate the legality and profitability, I might write this as:
get gep indices
visit gep indices, checking for constants

Your current version might be slightly faster, but it's IMO less clear.

Minor.  I'll defer to your preference.  

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:623
@@ +622,3 @@
+      Value *Replacement = Builder.CreateGEP(GEPBase, makeArrayRef(OffsetV));
+      cast<Instruction>(Replacement)->removeFromParent();
+      cast<Instruction>(Replacement)->insertAfter(GEPBase);
----------------
I'd suggest casting once on the previous line.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:624
@@ +623,3 @@
+      cast<Instruction>(Replacement)->removeFromParent();
+      cast<Instruction>(Replacement)->insertAfter(GEPBase);
+      Replacement->takeName(ToReplace);
----------------
I'm confused by the insertion placement games.  Why isn't inserting the new GEP right after the relocation you're replacing okay?

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:674
@@ +673,3 @@
+  for (auto &Item : RelocateInstMap)
+    AffectRelocateReplacements(Item.first, Item.second, MadeChange);
+  return MadeChange;
----------------
Giving names to Item.first and Item.second would make this code more clear.  Temporary variables could be used.  Creating a helper struct rather than a pair would also be fine.

http://reviews.llvm.org/D6883

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