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

[Sanjoy Das]

REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1418
@@ +1417,3 @@
+
+    assert(AllocaMap.count(OriginalValue));
+    Value *Alloca = AllocaMap[OriginalValue];
----------------
Nit: change this to `assert(AllocaMap.count() && "why this should be true!")`.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1422
@@ +1421,3 @@
+    StoreInst *Store = new StoreInst(RematerializedValue, Alloca);
+    Store->insertAfter(cast<Instruction>(RematerializedValue));
+
----------------
Unless you have a reason to prefer otherwise, I'd push this invariant of `RematerializedValue` being an `Instruction` earlier and

```
typedef DenseMap<Value *, Instruction *> RematerializedValueMapTy;
```

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1456
@@ +1455,3 @@
+                                        F.getEntryBlock().getFirstNonPHI());
+    allocaMap[LiveValue] = Alloca;
+    PromotableAllocas.push_back(Alloca);
----------------
Variable naming in this function is somewhat haphazard.  Do you mind bringing the variable names up to LLVM's coding standards for the functions you touched in a later separate NFC change, (once this change is in)?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1471
@@ +1470,3 @@
+      Value *OriginalValue = RematerializedValuePair.second;
+      if (allocaMap.count(OriginalValue) != 0) {
+        continue;
----------------
Most of LLVM consistently avoids bracing small `if` bodies like these, can you change this to

```
if (...)
  continue;
```
here and elsewhere?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1801
@@ +1800,3 @@
+// values are visited (currently it is GEP's and casts). Returns true if it
+// sucesfully reached "BaseValue" and false otherwise.
+// Fills "ChainToBase" array with all visited values. "BaseValue" is not
----------------
Nit: should be "successfully"

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1842
@@ +1841,3 @@
+                       TargetTransformInfo *TTI) {
+  assert(TTI);
+
----------------
If `TTI` is non-null, then take a reference instead of a pointer.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1847
@@ +1846,3 @@
+  for (Instruction *Instr: Chain) {
+    assert(isa<GetElementPtrInst>(Instr) || isa<CastInst>(Instr));
+
----------------
I'd structure this as:

```
if (CastInst *CI = ...) {
} else if (GEPInst *GEP = ...) {
} else
  llvm_unreachable("message");
```

Currently the `continue` statements are redundant and they make it look like that there is some fallthrough logic that you're trying to avoid, but there isn't.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1850
@@ +1849,3 @@
+    if (CastInst *CI = dyn_cast<CastInst>(Instr)) {
+      assert(CI->isNoopCast(CI->getModule()->getDataLayout()));
+
----------------
Add a string to the `assert` on why the condition should be true.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1873
@@ +1872,3 @@
+
+  // Avoid rematerializing very long instruction chains to avoid code size
+  // problems
----------------
What is a situation where this guard will help?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1887
@@ +1886,3 @@
+                                    PartiallyConstructedSafepointRecord &Info,
+                                    TargetTransformInfo *TTI) {
+  assert(TTI);
----------------
If `TTI` is never null then take `TargetTransformInfo &` instead.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1893
@@ +1892,3 @@
+  SmallVector<Value *, 64> LiveSet;
+  LiveSet.insert(LiveSet.begin(), Info.liveset.begin(), Info.liveset.end());
+
----------------
I'd rather you not do this, but instead accumulate the values you're supposed to delete into a `SmallVector` and batch-delete them after the loop.

If you wish to keep this as it is, please use the `SmallVector` constructor that takes a begin and end iterator.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1914
@@ +1913,3 @@
+    }
+    // If it's too expensive - skip it
+    if (Cost >= RematerializationThreshold)
----------------
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.

================
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));
----------------
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?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1945
@@ +1944,3 @@
+          assert(LastValue);
+          ClonedValue->replaceUsesOfWith(LastValue, LastClonedValue);
+        }
----------------
In debug mode assert that `ClonedValue` does not use any of the values in `ChainToBase` except `LastValue`.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1966
@@ +1965,3 @@
+
+      Instruction *NormalInsertBefore = cast<InvokeInst>(CS.getInstruction())->
+          getNormalDest()->getFirstInsertionPt();
----------------
Please common out the `cast<InvokeInst>(CS.getInstruction())`.  That way the `assert(CS.isInvoke())` also becomes redundant.

http://reviews.llvm.org/D9774

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