[PATCH] D22080: [CFLAA] Simplify CFLGraphBuilder by removing InstantiatedRelations and InstantiatedAttrs

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

As always, thanks for the patch!


================
Comment at: lib/Analysis/CFLGraph.h:42
@@ -56,3 +41,3 @@
   struct Edge {
-    EdgeType Type;
+    int64_t Offset;
     Node Other;
----------------
Can you talk about the purpose of `Offset`, please? It looks like it's unused, and only ever has a value of 0. Will this change in the future, or am I missing something?

================
Comment at: lib/Analysis/CFLGraph.h:54
@@ +53,3 @@
+  class ValueInfo {
+  private:
+    std::vector<NodeInfo> Levels;
----------------
Redundant private

================
Comment at: lib/Analysis/CFLGraph.h:62
@@ +61,3 @@
+        return false;
+      while (NumLevels <= Level) {
+        Levels.push_back(NodeInfo{EdgeList(), AliasAttrs()});
----------------
Can this just be `Levels.resize(Level+1)`?

================
Comment at: lib/Analysis/CFLGraph.h:89
@@ -87,2 +88,3 @@
       return nullptr;
-    return &Itr->second;
+    else if (Itr->second.getNumLevels() <= N.DerefLevel)
+      return nullptr;
----------------
Please merge this with the condition above

================
Comment at: lib/Analysis/CFLGraph.h:97
@@ -93,1 +96,3 @@
+      return nullptr;
+    else if (Itr->second.getNumLevels() <= N.DerefLevel)
       return nullptr;
----------------
This, too

================
Comment at: lib/Analysis/CFLGraph.h:164
@@ -182,1 +163,3 @@
+    SmallPtrSet<const GlobalValue *, 8> GlobalValues;
+    SmallPtrSet<const ConstantExpr *, 8> ConstantExprs;
 
----------------
Is there a reason we have separate sets for globals vs constants?

================
Comment at: lib/Analysis/CFLGraph.h:192
@@ +191,3 @@
+      if (auto GVal = dyn_cast<GlobalValue>(Val)) {
+        if (!GlobalValues.insert(GVal).second)
+          return;
----------------
Why can't we just say `if (!Graph.addNode({Val, 0}) return;` (here and for the CExpr case)?

================
Comment at: lib/Analysis/CFLGraph.h:194
@@ +193,3 @@
+          return;
+        Graph.addNode(InstantiatedValue{GVal, 0});
+        Graph.addAttr(InstantiatedValue{GVal, 0},
----------------
It looks like `addNode(Foo)` + `addAttr(Foo, A)` is a common pattern. Maybe we could either add a `WithAttrs` param (or whatever) to `addNode`, or make an `addNodeWithAttrs` method, so we don't need to do redundant lookups?

================
Comment at: lib/Analysis/CFLGraph.h:209
@@ +208,3 @@
+    void addNode(Value *Val, AliasAttrs Attr = AliasAttrs()) {
+      checkGlobalOrCExpr(Val);
+      Graph.addNode(InstantiatedValue{Val, 0});
----------------
If `Val` turns out to be a global or constant expression, it seems mildly redundant to add it (or see that we've already added it) in `checkGlobalOrCExpr`, then try to re-add it below.

Also, given its uses, I think `addGlobalOrCExpr` may be a better name. :)

================
Comment at: lib/Analysis/CFLGraph.h:218
@@ -229,5 +217,3 @@
         return;
-      addNode(From);
-      if (To != From)
-        addNode(To);
-      Graph.addEdge(From, To, Type);
+      checkGlobalOrCExpr(From);
+      Graph.addNode(InstantiatedValue{From, 0});
----------------
And here

================
Comment at: lib/Analysis/CFLGraph.h:221
@@ +220,3 @@
+      if (To != From) {
+        checkGlobalOrCExpr(To);
+        Graph.addNode(InstantiatedValue{To, 0});
----------------
And here

================
Comment at: lib/Analysis/CFLGraph.h:392
@@ -385,2 +391,3 @@
       for (Value *V : CS.args())
-        addNode(V);
+        if (V->getType()->isPointerTy())
+          addNode(V);
----------------
Is there a case where we add non-pointer types to the graph? If not, can we either add `assert(isPointerTy)` to `addNode`, or add this logic into `addNode`?

It looks like it's really easy to miss a type check at the moment. :/

================
Comment at: lib/Analysis/CFLSteensAliasAnalysis.cpp:203
@@ -222,9 +202,3 @@
   auto &Graph = GraphBuilder.getCFLGraph();
-  SmallVector<Value *, 16> Worklist;
-  for (auto Node : Graph.nodes())
-    Worklist.push_back(Node);
-
-  while (!Worklist.empty()) {
-    auto *CurValue = Worklist.pop_back_val();
-    SetBuilder.add(CurValue);
-    if (canSkipAddingToSets(CurValue))
+  for (const auto &mapping : Graph.value_mappings()) {
+    auto Val = mapping.first;
----------------
s/mapping/Mapping


http://reviews.llvm.org/D22080