[PATCH] [RewriteStatepointsForGC] Use an actual liveness algorithm

[Sanjoy Das]

I think this is okay to check in as-is and the review comments addressed post-commit.  I have a mild preference running `clang-format` on the diff pre-checkin (that way the history stays clean), but I'd be okay with that done post-commit as well (maybe over the entire file even).


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:93
@@ +92,3 @@
+  /// Values used in this block (and thus live).  Exclusive of this in KillSet.
+  DenseMap<BasicBlock *, DenseSet<Value *>> LiveSet;
+  
----------------
I don't understand the comment "Exclusive of this in KillSet.".

Also, is this a subset of `LiveIn`?  Or do you not count `X` in cases like

```
  X = def()
  use(X)
```


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1937
@@ +1936,3 @@
+/// the live-out set of the basic block
+static void computeGCPtrLiveness(BasicBlock::reverse_iterator rbegin,
+                                 BasicBlock::reverse_iterator rend,
----------------
Why not call this `ComputeLiveInValues` or something like that?  `computeGCPtrLiveness` is somewhat vague.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1953
@@ +1952,3 @@
+    // USE - Add to the LiveIn set for this instruction
+    for (unsigned i = 0; i < I->getNumOperands(); i++) {
+      Value *V = I->getOperand(i);
----------------
I think you can just do `for (Value *V : I->operands()) {`

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1967
@@ +1966,3 @@
+static void computeLiveOutSeed(BasicBlock *BB,
+                               DenseSet<Value *> &LiveTmp) {
+
----------------
The whitespace here (and elsewhere) looks weird.  Please run this change through `clang-format` before checking in.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2022
@@ +2021,3 @@
+  checkBasicSSA(DT, Data.LiveSet[&BB], BB.getTerminator());
+  checkBasicSSA(DT, Data.LiveOut[&BB], BB.getTerminator(), true);
+  checkBasicSSA(DT, Data.LiveIn[&BB], BB.getTerminator());
----------------
Minor: I'd pass in `true` for `TermOkay` only if it was a GC pointer typed `invoke`, to make the checks more precise.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2029
@@ +2028,3 @@
+
+  DenseSet<BasicBlock *> WorklistSet;
+  SmallVector<BasicBlock *, 200> Worklist;
----------------
Can we use a `SetVector` here?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2046
@@ +2045,3 @@
+    Data.KillSet[&BB] = computeKillSet(&BB);
+    Data.LiveSet[&BB] = DenseSet<Value *>();
+    computeGCPtrLiveness(BB.rbegin(), BB.rend(), Data.LiveSet[&BB]);
----------------
Minor: I'd use `.clear()` here.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2076
@@ +2075,3 @@
+    }
+    // assert OutLiveOut is a subset of LiveOut
+    if (OldLiveOutSize == LiveOut.size()) {
----------------
? Please actually add an assert. :)

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2092
@@ +2091,3 @@
+    const DenseSet<Value *> &OldLiveIn = Data.LiveIn[BB];
+    // assert: OldLiveIn is a subset of LiveTmp
+    if (OldLiveIn.size() != LiveTmp.size()) {
----------------
?  Please actually add an assert. :)

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2127
@@ +2126,3 @@
+
+static void fixupLiveness(GCPtrLivenessData &RevisedLivenessData,
+                          const CallSite &CS,
----------------
`fixup` is too generic, IMO.  Maybe calls this `AdjustLivenessForBasePointers` or something similar?

http://reviews.llvm.org/D8674

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