[PATCH] Move easily derivable pointers optimization from CodeGenPrepare to RewriteStatepointPass

[Sanjoy Das]

LGTM with comments addressed.


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1860
@@ +1859,3 @@
+      Type *ValTy = GEP->getPointerOperandType()->getPointerElementType();
+      bool IsComplex = !GEP->hasAllConstantIndices();
+      Cost += TTI->getAddressComputationCost(ValTy, IsComplex);
----------------
I'm not sure that this is the right meaning of `IsComplex`.  Even with variable indices x86 can usually merge the GEP into the load/store.  I think `IsComplex` should be "has more than one non-constant operand".

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1873
@@ +1872,3 @@
+
+  // Avoid rematerializing very long instruction chains to avoid code size
+  // problems
----------------
igor-laevsky wrote:
> sanjoy wrote:
> > What is a situation where this guard will help?
> In theory TTI functions can return zero cost. In that case it is possible to have very long instruction chain with estimated cost of zero. It does not seems very right. This is purely hypothetical and I don't think it is  very likely to happen on practice.
But I'd say in those cases it should be perfectly okay to have a large chain.  Otherwise TTI is wrong. :)

Actually, I don't have strong objections to this part, but I think it is more sensible to have this as an optimization to reduce //compile time// -- in the caller, check if `Chain.size()` is higher than some threshold (`10` is good) and if so bail out of rewriting anything for the given `Chain`.

In either case, please add a test case that exercises this path.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1914
@@ +1913,3 @@
+    }
+    // If it's too expensive - skip it
+    if (Cost >= RematerializationThreshold)
----------------
igor-laevsky wrote:
> sanjoy wrote:
> > No need to fix this now, but add a `TODO` for adjusting the cost when recomputing the derived pointer will let you remove the computation of the unrelocated derived pointer in cases where the only use of the derived pointer was the statepoint.
> Not sure if I am following.
> 
> Are you saying about cases when we will be able to remove instruction chain (or part of it) later? I.e it also can happen if there was several intersecting instruction chains we have rematerialized.
I'm talking about the case where we go from:

```
%D = GEP %B, 4
; No use of %D before statepoint
(%D2, %B2) = statepoint_and_relocate(%D, %B)
```

to


```
%B2 = statepoint_and_relocatt(%B)
%D2 = GEP %B2, 4

```

Here we have only moved the GEP, but did not copy it, so we did not add any cost to the computation overall.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1934
@@ +1933,3 @@
+        // Only GEP's and casts are suported as we need to be careful to not
+        // introduce any new uses of pointers not in the liveset.
+        assert(isa<GetElementPtrInst>(Instr) || isa<CastInst>(Instr));
----------------
igor-laevsky wrote:
> sanjoy wrote:
> > But you may still semantically "promote" a live value that was solely the base pointer for another derived pointer (and had no direct uses after the safepoint) to a pointer that is fundamentally live over the safepoint (has a direct use after the safepoint), right?  Will that always work correctly?
> Yes, it should work. Nowhere in this pass we distinguish pointers which are live "for real" and base pointer which are required to be live because of the derived pointer, but does not have uses of their own.
Great!  Please add one or two lines explaining this.  If you can figure out a way to `assert` on this then even better.

================
Comment at: test/Transforms/RewriteStatepointsForGC/basics.ll:2
@@ -1,3 +1,3 @@
 ; This is a collection of really basic tests for gc.statepoint rewriting.
-; RUN:  opt %s -rewrite-statepoints-for-gc -S | FileCheck %s
+; RUN:  opt %s -rewrite-statepoints-for-gc -spp-rematerialization-threshold=0 -S | FileCheck %s
 
----------------
Very minor: I'd use `-spp-rematerialization-threshold=-1` here to make it obvious that you're preventing any rewriting, even zero-cost ones.

================
Comment at: test/Transforms/RewriteStatepointsForGC/rematerialize-derived-pointers.ll:12
@@ +11,3 @@
+  %ptr = getelementptr i32, i32 addrspace(1)* %base, i32 15
+  ; CHECK: getelementptr
+  %sp = call i32 (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @do_safepoint, i32 0, i32 0, i32 0, i32 0)
----------------
Please add some (2-3) more detailed test cases that check that you've generated the correct GEPs and cast instructions.

http://reviews.llvm.org/D9774

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