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

[Jia Chen via llvm-commits]

grievejia added inline comments.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:394
@@ -421,1 +393,3 @@
         return false;
+      if (Fn->arg_size() > CS.arg_size())
+        return false;
----------------
george.burgess.iv wrote:
> Please add a comment explaining why this check exists.
It used to be "if (Fn->arg_size() == CS.arg_size()) return false", and I thought I asked you for justifications some time ago... I just kept what was there and relaxed it a little bit.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:775
@@ +774,3 @@
+        auto MaybeRelation =
+            getIndexRelation(Sets, MainInfo->Index, SubInfo->Index);
+        if (!MaybeRelation.hasValue())
----------------
george.burgess.iv wrote:
> 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.
Thanks for the note. 

I am aware of that. The reason I invoked hasValue() is consistency: I saw "XXX.hasValue()" at several other places in the same file. 

================
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
 
----------------
george.burgess.iv wrote:
> 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)
Oops sorry about that. My editor auto-wrap long comments for C++ source files (thanks to clang-format), but I forgot it didn't do the same thing for .ll files :( 


http://reviews.llvm.org/D21475