[PATCH] LLVM CFL Alias Analysis -- Supporting Data Structures

[Nick Lewycky]

For StratifiedSets, I need more documentation (*cough* or to read the paper) in order to continue reviewing it.

WeightedBidirectedGraph.h looks good to me, though I'd prefer "Bidirectional" (optional).

================
Comment at: lib/Analysis/StratifiedSets.h:9
@@ +8,3 @@
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_ADT_STRATIFIEDSETS_H
----------------
Missing high-level conceptual description of stratified sets.

================
Comment at: lib/Analysis/StratifiedSets.h:37
@@ +36,3 @@
+
+// The number of attributes that StratifiedAttrs should contain
+static constexpr unsigned NumStratifiedAttrs = 32;
----------------
Period to end complete sentence.

Also, the reader is left wondering what attributes are and why it should contain 32?

================
Comment at: lib/Analysis/StratifiedSets.h:171-173
@@ +170,5 @@
+// together. So, we merge the set that contains %a with the set that contains
+// %b. We then  recursively merge the set above %a with the set above %b, and
+// the set below  %a with the set below %b, etc. Ultimately, the sets for this
+// program would  end up like:  {%ad}, {%a, %b, %k}, {%bp}, where {%ad} is below
+// {%a, %b, %c} is below {%ad}.
----------------
Extra space between then and recursively.
Extra space between below and %a.
Extra space between would and end.


================
Comment at: lib/Analysis/StratifiedSets.h:338
@@ +337,3 @@
+  // Link.externals will be set in all Link.externals "below" it.
+  static void propogateAttrs(std::vector<StratifiedLink> &Links) {
+    const auto getHighestParentAbove = [&Links](StratifiedIndex Idx) {
----------------
propogateAttrs -> propagateAttrs

================
Comment at: lib/Analysis/StratifiedSets.h:389
@@ +388,3 @@
+
+  bool addAbove(const T &Main, const T &ToAdd) {
+    assert(has(Main));
----------------
These methods need docstrings.

================
Comment at: lib/Analysis/StratifiedSets.h:396
@@ +395,3 @@
+    auto Above = linksAt(Index).getAbove();
+    return addAtMerging(ToAdd, Above);
+  }
----------------
To check my understanding, this is what causes a cycle (attempting to insert A above B given that B is already above A) to be flattened into a single, uh, "stratus"?


================
Comment at: lib/Analysis/WeightedBidirectedGraph.h:1
@@ +1,2 @@
+//===- WeightedBidirectedGraph.h - Abstract weighted, bidirected graph. ---===//
+//
----------------
"Bidirected"? Bidirectional?

================
Comment at: lib/Analysis/WeightedBidirectedGraph.h:1
@@ +1,2 @@
+//===- WeightedBidirectedGraph.h - Abstract weighted, bidirected graph. ---===//
+//
----------------
Nick Lewycky wrote:
> "Bidirected"? Bidirectional?
I'm not convinced this should be pulled out of CFL Alias Analysis.

LLVM's graph infrastructure has:
 a) generic algorithms that operate over child_begin/child_end iterators
 b) wrappers that are constructed from other objects and provide child_begin/end.
Sometimes the wrappers in (b) are null wrappers the implementation of the graph APIs are intrusive upon the underlying classes.

This doesn't fit that model at all, this is a graph data type. If this needs to exist at all (ie., we can't lazily construct it from other objects we have handy like the IR) then it should be local to CFLAliasAnalysis. That could mean creating a new CFGAlias directory to put this file in.

http://reviews.llvm.org/D4550