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

[Philip Reames]

Lots of cleanup (both style and correctness) needed, but I like the direction.  Please revise.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:550
@@ +549,3 @@
+// %val = load %ptr'
+bool CodeGenPrepare::SimplifyOffsetableRelocate(BasicBlock &BB)
+{
----------------
First, thank you for working on this!

Second, I'm not entirely sure this is the right place for this.  It's possible that having this in InstCombine (alternatively or possible also) might open up IR level optimizations based on comparisons of relocated pointers.  Thoughts?  (Also, this is not a reason to hold your current patch.)

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:556
@@ +555,3 @@
+    if (CallInst *CI = dyn_cast<CallInst>(&I)) {
+      IntrinsicInst *II = dyn_cast<IntrinsicInst>(CI);
+      if (II && II->getIntrinsicID() == Intrinsic::experimental_gc_relocate)
----------------
You can combine these two tests into one.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:561
@@ +560,3 @@
+  }
+  if (RelocateCalls.size() < 2)
+    return false;
----------------
Add a comment about why 2 is the magic number.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:564
@@ +563,3 @@
+  // Assume that the first relocate in the basic block is relocating the base
+  // object. TODO: what can be done to get rid of this assumption?
+  IntrinsicInst *MasterRelocate = RelocateCalls[0];
----------------
sanjoy wrote:
> This seems limiting, but correct; and a strict improvement over what we have.  We could easily have a `DenseMap<CallInst *, CallInst *>` that we build in a first pass that maps derived gc relocates to base gc relocates; and write the rest of the algorithm in terms of that.
Er, maybe I'm misreading Sanjoy's comment, but I think his first sentence is wrong.  There is no guarantee that the first gc.relocate encountered is a base relocation,

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:570
@@ +569,3 @@
+  unsigned BaseIdx =
+    dyn_cast<ConstantInt>(MasterRelocate->getArgOperand(1))->getZExtValue();
+  if (BaseIdx !=
----------------
sanjoy wrote:
> Don't use `dyn_cast<>` unless you want to check if the cast succeeded or not.  I think a `cast<>` is more appropriate here.  Same applies to some later calls to `dyn_cast`.
Take a look at Statepoint.h for useful utility routines here.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:579
@@ +578,3 @@
+  Value *Base = MasterStatepoint->getArgOperand(BaseIdx);
+  for (IntrinsicInst *II : RelocateCalls) {
+    if (dyn_cast<IntrinsicInst>(II->getArgOperand(0)) != MasterStatepoint ||
----------------
Organizing this as visiting each of the uses of a statepoint, and then visiting each statepoint individually would make this strictly stronger.  Also, vastly preferred from a readability perspective. 

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:588
@@ +587,3 @@
+      // A duplicate relocate call? Won't an earlier optimization coalesce such
+      // duplicates?
+      continue;
----------------
Not yet.  But if you wanted to write an InstCombine, I'd be happy to review it...

http://reviews.llvm.org/D6883

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