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

[George Burgess IV via llvm-commits]

george.burgess.iv added inline comments.

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

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:394
@@ -421,1 +393,3 @@
         return false;
+      // Try to be defensive if the caller does not provide enough arguments
+      if (Fn->arg_size() > CS.arg_size())
----------------
> I thought I asked you for justifications some time ago

You did :)

IIRC, the best I could offer you was "we somehow devirtualized the function pointer to N different functions, and their arities didn't match up." Given that that's the best explanation we have (and if we ever do anything fancy with indirect calls, it's probably going to be the resolver's job to ensure that the potential callee makes sense to call), I'm tempted to turn this into an assert, rather than guarding against this case without knowing why. You can blame me if it ends up breaking anything. :)

It also looks like this change would also make us do interprocedural analysis on variadic functions -- I think that should be fine, but can we have a test for that, please? If it helps, here's a link to vararg stuff: http://llvm.org/docs/LangRef.html#variable-argument-handling-intrinsics

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:418
@@ +417,3 @@
+          // Therefore by the time we reach here, FromVal and ToVal should
+          // already exist in the graph. We can go ahead and add them directly
+          Graph.addEdge(FromVal, ToVal, EdgeType::Assign);
----------------
Please add a period at the end of the comment.

================
Comment at: test/Analysis/CFLAliasAnalysis/interproc-ret-arg.ll:7
@@ +6,3 @@
+
+; We have to xfail this since @return_arg_callee is treated as an opaque function, and the anlysis couldn't prove that %b and %c are not aliases 
+; XFAIL: *
----------------
80 chars, please


http://reviews.llvm.org/D21475