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

[Jia Chen via llvm-commits]

grievejia marked 6 inline comments as done.

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:120
@@ +119,3 @@
+    else
+      return make_range<const_valuestate_iterator>(Itr->second.begin(),
+                                                   Itr->second.end());
----------------
george.burgess.iv wrote:
> 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.
I thought so in the beginning. But if we choose not to sort A and B and just let the insertion happen, then I'm pretty sure that half of the storage in ReachSet will be wasted, since we will store the fact that "A is reachable from B" twice. 

In your case, we won't put A in B's reaching set, but we do put B in A's reaching set, so if the fixpoint is properly reached, then there shouldn't be any problem because we will eventually propagate everything and it doesn't matter whether the reachability goes through A or B. The question is, will the propagation terminate before a fixpoint is reached? That I'm not sure. More tests are needed to help me figure that out.

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:198
@@ +197,3 @@
+    // Sort AliasList for faster lookup
+    std::sort(AliasList.begin(), AliasList.end());
+  }
----------------
george.burgess.iv wrote:
> `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?
Wow I thought std::sort uses std::less by default! What a wonderful design C++ has!

In practices I think I could just use operator< for pointer comparison. Most compiler would just accept it. The reason I used std::less is to add some portability here because strictly speaking comparing pointers to different allocations is UB according to language spec. 

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:255
@@ +254,3 @@
+    Ret = NodeBelow;
+  return Ret;
+}
----------------
george.burgess.iv wrote:
> Maybe:
> 
> ```
> if (...)
>   return NodeBelow;
> return None;
> ```
> 
> would be a bit cleaner than `Optional<...> Ret;`?
Agreed. I wrote it this way because I am pretty sure NRVO can be triggered here. I'm not too sure if today's compiler is smart enough to RVO if different values are returned in different branches?

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:305
@@ +304,3 @@
+                WorkList);
+    if (auto AliasSet = MemSet.getMemoryAliases(ToNode)) {
+      for (const auto &MemAlias : *AliasSet)
----------------
george.burgess.iv wrote:
> 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.
Agreed. It shouldn't matter, and I hope it doesn't...


https://reviews.llvm.org/D22291