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

[Ramkumar Ramachandra]

Expect a new iteration immediately.


================
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,
----------------
reames wrote:
> llvm style is computeBase...
> 
> Make the param SmallVectorImpl so that it's size independent.
I see `static bool SinkCast`, `static bool OptimizeNoopCopyExpression`, `static bool OptimizeCmpExpression`, but I suppose it's good to follow the documented convention when writing new functions? `simplifyOffsetableRelocate` it is.

Note: I can't do `SmallVectorImpl` inside the `DenseMap`.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:594
@@ +593,3 @@
+// computes a replacement, and affects it.
+static void AffectRelocateReplacements(IntrinsicInst *GEPBase,
+                                       const SmallVector<IntrinsicInst *, 2>
----------------
reames wrote:
> I think GEPBase might be more clearly named "RelocatedBase".
> 
> Also, "affect" is a slightly odd verb.  Possibly: SimplifyOneRelocate?
`SimplifyOneRelocate` is misleading because we're simplifying multiple derived-pointer relocates corresponding to the same base-pointer relocate. Perhaps `SimplifyRelocatesOffABase`?

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:616
@@ +615,3 @@
+      SmallVector<Value *, 2> OffsetV;
+      if (!GetGEPSmallConstantIntOffsetV(Derived, OffsetV))
+        return;
----------------
reames wrote:
> 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.  
Get - GEP - Small - ConstantInteger - OffsetVector. I thought the name was clever and clear; my preference is not to change it.

Also, I found a bug: it shouldn't `return` if it couldn't find a small constant-integer offset-vector; it should `continue`, so that other derived pointer relocations get a chance.

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

```
       %ptr = getelementptr i32* %base, i32 15
       %tok = call i32 (i1 ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_i1f(i1 ()* @return_i1, i32 0, i32 0, i32 0, i32* %base, i32* %ptr)
       %ptr-new = call i32* @llvm.experimental.gc.relocate.p0i32(i32 %tok, i32 4, i32 5)
       %base-new = call i32* @llvm.experimental.gc.relocate.p0i32(i32 %tok, i32 4, i32 4)
       %ret = load i32* %ptr-new
```

Here, if I replace the `%ptr-new` line with a `gep`, against whom am I gep'ing? `%base-new` ofcourse, but that's on the next line! The problem is therefore to reorder instructions so that the new instruction comes after `%base-new`, no matter what the case.

Makes sense?

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:674
@@ +673,3 @@
+  for (auto &Item : RelocateInstMap)
+    AffectRelocateReplacements(Item.first, Item.second, MadeChange);
+  return MadeChange;
----------------
reames wrote:
> 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.
Hm? It's a pair because that's what you get when iterating over a `DenseMap`. I don't want to create new variables, so I've written comments about what `Item.first` and `Item.second` are, as a compromise.

http://reviews.llvm.org/D6883

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