[PATCH] Add a pass for constructing gc.statepoint sequences w/explicit relocations

[Sanjoy Das]

Overall, the main issues are obvious stylistic issues.  Specifically:

- using LLVM-specific data structures, like `DenseSet` and `DenseMap` instead of the `std::` counterparts.
- using LLVM variable and function naming conventions.
- using C++ style comments

I have some small nits inline representative of the three points above.

My two cents is that you should get some of the low-hanging fruit in terms of the above three aspects before checking in, and then fix the rest incrementally once the pass is in tree.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:13
@@ +12,3 @@
+//
+//===----------------------------------------------------------------------===//
+
----------------
An example here will make what this pass does clearer.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:60
@@ +59,3 @@
+// look like normal behavior.
+//#define USING_BUGPOINT
+
----------------
I this this should be removed before checking in.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:90
@@ +89,3 @@
+namespace {
+/** The type of the internal cache used inside the findBasePointers family
+    of functions.  From the callers perspective, this is an opaque type and
----------------
C++ style comments please.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:105
@@ +104,3 @@
+namespace {
+struct PartiallyConstructedSafepointRecord {
+  /// The set of values known to be live accross this safepoint
----------------
The field names of this struct don't follow the usual `CamelCaseStartingWithUpperCase` llvm style.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:107
@@ +106,3 @@
+  /// The set of values known to be live accross this safepoint
+  std::set<llvm::Value *> liveset;
+
----------------
`DenseSet`?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:110
@@ +109,3 @@
+  /// Mapping from live pointers to a base-defining-value
+  std::map<llvm::Value *, llvm::Value *> base_pairs;
+
----------------
`DenseMap`?  Also applies to other fields in this `struct`.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:161
@@ +160,3 @@
+  } else if (StructType *ST = dyn_cast<StructType>(Ty)) {
+    bool bad = false;
+    for (Type *SubType : ST->subtypes())
----------------
LLVM style says `bool Bad`.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:169
@@ +168,3 @@
+
+/** Conservatively identifies any definitions which might be live at the
+    given instruction. The  analysis is performed immediately before the
----------------
C++ style comments please.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:214
@@ +213,3 @@
+    assert(DT.dominates(BBI, pred));
+    assert(isPotentiallyReachable(BBI, pred) &&
+           "dominated block must be reachable");
----------------
If we pass in `DT` we can potentially get a stronger assertion.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:262
@@ +261,3 @@
+
+static bool order_by_name(llvm::Value *a, llvm::Value *b) {
+  if (a->hasName() && b->hasName()) {
----------------
I think this can be

```
if (a->hasName() || b->hasName())
  return a->getName() < b->getName();
return a < b;
```

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:392
@@ +391,3 @@
+      // If the frontend marked this as a known base pointer...
+      if (i2p->getMetadata("verifier_exception")) {
+        return def;
----------------
This needs to be documented.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:454
@@ +453,3 @@
+  }
+  // Let's assume that any call we see is to a java function.  Java
+  // functions can only return Java objects (i.e. base pointers).
----------------
I don't think this is Java specific -- the language should be changed to reflect that.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:499
@@ +498,3 @@
+
+  // The last two cases here don't return a base pointer.  Instead, they
+  // return a value which dynamically selects from amoung several base
----------------
I'd be more comfortable if this function explicitly returned a sum type or something like that.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:512
@@ +511,3 @@
+  I->dump();
+  assert(false && "unknown type");
+  return nullptr;
----------------
Use `llvm_unreachable`.  Also, can we do `errs() << "unknown type: " << *I << "\n"`?

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:681
@@ +680,3 @@
+
+  /* Here's the rough algorithm:
+     - For every SSA value, construct a mapping to either an actual base
----------------
C++ style comments please.

http://reviews.llvm.org/D6975

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