[PATCH] D21475: [CFLAA] Summarize interprocedural aliasing information instead of recomputing it at callsite

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Thanks for the patch, and for adding all of the tests! :)

Looks good with a handful of nits.


================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:394
@@ -421,1 +393,3 @@
         return false;
+      if (Fn->arg_size() > CS.arg_size())
+        return false;
----------------
Please add a comment explaining why this check exists.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:413
@@ +412,3 @@
+            (ToIndex == 0) ? CS.getInstruction() : CS.getArgument(ToIndex - 1);
+        if (FromVal->getType()->isPointerTy() &&
+            ToVal->getType()->isPointerTy())
----------------
Please add a comment to the effect of "FromVal and ToVal should already exist in the graph, so we can add this edge directly"

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:745
@@ +744,3 @@
+  for (auto &Param : Fn.args()) {
+    if (Param.getType()->isPointerTy()) {
+      ParamInfos.push_back(Sets.find(&Param));
----------------
Nit: Remove these braces, please.

(Braces on the loop are fine IMO)

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:751
@@ +750,3 @@
+  // Collect StratifiedInfo for each return value
+  SmallVector<Optional<StratifiedInfo>, 4> RetInfos;
+  for (unsigned I = 0, E = RetVals.size(); I != E; ++I)
----------------
Nit: Reserve `RetVals.size()` elements ahead of time, please

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:763
@@ +762,3 @@
+
+      /// Adding edges between arguments for arguments that may end up aliasing
+      /// each other. This is necessary for functions such as
----------------
Nit: `///` -> `//`

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:775
@@ +774,3 @@
+        auto MaybeRelation =
+            getIndexRelation(Sets, MainInfo->Index, SubInfo->Index);
+        if (!MaybeRelation.hasValue())
----------------
If you want to simplify things a bit, `Optional<T>` has an implicit conversion to `bool`, so you can do `if (getIndexRelation(...)) RetParamRelations.push_back(...);`.

Entirely up to you, though.

================
Comment at: test/Analysis/CFLAliasAnalysis/basic-interproc.ll:1
@@ -1,3 +1,2 @@
-; This testcase ensures that CFL AA gives conservative answers on variables
-; that involve arguments.
+; This testcase ensures that CFL AA won't be too conservative when trying to do interprocedural analysis on simple callee
 
----------------
Nit: Please wrap comments at 80 chars, here and elsewhere.

(It's fine to leave the run lines at > 80 chars if you'd like)


http://reviews.llvm.org/D21475