[PATCH] D22291: [CFLAA] Added an initial working implementation of Andersen's analysis

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Thanks for the patch!


================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:119
@@ +118,3 @@
+                                                   const_valuestate_iterator());
+    else
+      return make_range<const_valuestate_iterator>(Itr->second.begin(),
----------------
Redundant else

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:120
@@ +119,3 @@
+    else
+      return make_range<const_valuestate_iterator>(Itr->second.begin(),
+                                                   Itr->second.end());
----------------
Is this correct with how we handle `insert`?

ISTM that the assert wouldn't fire in the following code:

```
InstantiatedValue A{0x100, 0};
InstantiatedValue B{0x200, 0};

ReachSet.insert(A, B, MatchState::Whatever);
ReachSet.insert(B, A, MatchState::Whatever);
auto BAliases = ReachSet.reachableValueAliases(B);
assert(std::distance(BAliases.begin(), BAliases.end()) == 0);
```

...Which looks bad. I could be misunderstanding, though.

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:130
@@ +129,3 @@
+// We use AliasMemSet to keep track of all memory aliases (the nonterminal "M"
+// in the papter) during the analysis
+class AliasMemSet {
----------------
s/papter/paper. (Also, please add a period)

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:150
@@ +149,3 @@
+
+  const MemSet *getMemoryAliases(InstantiatedValue V) const {
+    auto Itr = MemMap.find(V);
----------------
Same "wouldn't insert make this not work" comment as in ReachabilitySet

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:154
@@ +153,3 @@
+      return nullptr;
+    else
+      return &Itr->second;
----------------
Redundant else

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:186
@@ +185,3 @@
+    // AliasMap only cares about top-level values
+    if (OuterMapping.first.DerefLevel > 0)
+      continue;
----------------
Please be consistent with line 193 when comparing against 0. :)

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:198
@@ +197,3 @@
+    // Sort AliasList for faster lookup
+    std::sort(AliasList.begin(), AliasList.end());
+  }
----------------
`sort` is only guaranteed to use `operator<` to sort its elems. Since we're using `std::less<const Value *>` elsewhere, do we want to use it here, as well?

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:216
@@ +215,3 @@
+
+  return std::binary_search(Itr->second.begin(), Itr->second.end(), RHS);
+}
----------------
Same comment as `sort` above

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:255
@@ +254,3 @@
+    Ret = NodeBelow;
+  return Ret;
+}
----------------
Maybe:

```
if (...)
  return NodeBelow;
return None;
```

would be a bit cleaner than `Optional<...> Ret;`?

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:305
@@ +304,3 @@
+                WorkList);
+    if (auto AliasSet = MemSet.getMemoryAliases(ToNode)) {
+      for (const auto &MemAlias : *AliasSet)
----------------
Looks like there's a fair amount of duplication between this and `FlowFromMemAlias` (same for `FlowTo`/`FlowToMemAlias`). We may want to clean that up at some point, assuming the order in which we propagate doesn't matter.

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:426
@@ +425,3 @@
+  auto &FunInfo = ensureCached(*Fn);
+  assert(FunInfo.hasValue());
+
----------------
FWIW, `Optional<T>::operator->` will do this assertion for you.

================
Comment at: lib/Analysis/CFLGraph.h:219
@@ -216,1 +218,3 @@
 
+    void addStoreEdge(Value *From, Value *To) {
+      assert(From != nullptr && To != nullptr);
----------------
I realize that there's a fair amount of duplication in this patch that we plan to kill, but it doesn't seem too difficult to merge this and `addLoadEdge`'s implementations.

(I'm fine with having two separate functions that users call; I just don't like two nearly identical functions sitting right next to each other. :) )


http://reviews.llvm.org/D22291