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

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Yay tests! Thanks for the patch


================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:99
@@ +98,3 @@
+typedef std::bitset<8> StateSet;
+LLVM_CONSTEXPR StateSet ReadOnlyStateMask =
+    (1 << static_cast<uint8_t>(MatchState::FlowFromReadOnly)) |
----------------
Is there a reason these aren't `static`?

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:256
@@ +255,3 @@
+    Index = Arg->getArgNo() + 1;
+  else if (std::find(RetVals.begin(), RetVals.end(), Val) != RetVals.end())
+    Index = 0;
----------------
Nit: Apparently we have a function called `is_contained` in STLExtras that does just this. So, can we use `llvm::is_contained(RetVals, Val))` instead, please? :)

================
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()) {
----------------
I realize that Args and RetVals will generally be small, but can we use a set (for either) instead, please?

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:303
@@ +302,3 @@
+  for (const auto &Arg : Fn.args()) {
+    if (std::find(RetVals.begin(), RetVals.end(), &Arg) != RetVals.end()) {
+      auto ArgVal = InterfaceValue{Arg.getArgNo() + 1, 0};
----------------
Nit: `is_contained`

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:339
@@ +338,3 @@
+          auto SrcIVal = InnerMapping.first;
+          if (hasReadOnlyState(InnerMapping.second))
+            ValueMap[SrcIVal.Val].FromRecords.push_back(
----------------
Do we intend to do nothing here if it InnerMapping is e.g. `FlowToReadWrite`?

================
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() {
----------------
Is this a cleanup from before, or is this as a result of this change? If the former, I'll commit it separately. :)


https://reviews.llvm.org/D22450