[PATCH] D22450: [CFLAA] Add interprocerual analysis support to CFLAndersAliasAnalysis

[Jia Chen via llvm-commits]

grievejia added inline comments.

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:302
@@ +301,3 @@
+  // needs special handling here.
+  for (const auto &Arg : Fn.args()) {
+    if (std::find(RetVals.begin(), RetVals.end(), &Arg) != RetVals.end()) {
----------------
george.burgess.iv wrote:
> I realize that Args and RetVals will generally be small, but can we use a set (for either) instead, please?
If the vectors are small, isn't it unlikely that sets will be faster?

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:339
@@ +338,3 @@
+          auto SrcIVal = InnerMapping.first;
+          if (hasReadOnlyState(InnerMapping.second))
+            ValueMap[SrcIVal.Val].FromRecords.push_back(
----------------
george.burgess.iv wrote:
> Do we intend to do nothing here if it InnerMapping is e.g. `FlowToReadWrite`?
Yes. All XXXReadWrite states are not useful here. 

The reason is, again, that (X, Y, XXXReadWrite) represent structures like "X = Z; Y = Z;". In other words, there is no transitive assignments from X to Y or from Y to X. If you reason about the inclusion relations, the points-to set of X does not have to be a subset of the points-to set of Y, and vice versa. It's therefore imprecise to add "X = Y" or "Y = X" to the function summary. 

What should be added to the summary is "X=Z" and "Y=Z", but we don't know what Z is so we can't do that. It is fine that we don't know Z though: if such a Z exists, we must have (X, Z, XXXReadOnly), (Z, X, XXXWriteOnly), (Y, Z, XXXReadOnly), and (Z, Y, XXXWriteOnly) all stored somewhere in ReachSet. Z will eventually be processed when those entries are encountered

================
Comment at: test/Analysis/CFLAliasAnalysis/Steensgaard/interproc-store-arg-multilevel.ll:32
@@ -31,3 +31,2 @@
 ; NoAlias: i32* %a, i32* %b
-; NoAlias: i32* %b, i32* %lp
 define void @test_store_arg_multilevel() {
----------------
george.burgess.iv wrote:
> Is this a cleanup from before, or is this as a result of this change? If the former, I'll commit it separately. :)
This is a cleanup. I made a mistake. %b must alias %lp here.


https://reviews.llvm.org/D22450