[PATCH] D21262: [CFLAA] Tag actual arguments as AttrEscaped instead of AttrUnknown

[Jia Chen via llvm-commits]

grievejia marked 2 inline comments as done.
grievejia added a comment.

In http://reviews.llvm.org/D21262#456704, @grievejia wrote:

> Also, let me rebase this patch to get rid of the changes that have already been applied to the patch that this one depends on (apparently there doesn't seem to have "phabricator rebase" functionality...).


Sorry, I meant "to include the changes that have already been...".


================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:158
@@ -153,3 +157,3 @@
 
-  Edge(Value *From, Value *To, EdgeType W, StratifiedAttrs A)
-      : From(From), To(To), Weight(W), AdditionalAttrs(A) {}
+  const EdgeList *getNode(Node N) const {
+    auto Itr = NodeImpls.find(N);
----------------
george.burgess.iv wrote:
> Looks like all the users of this fail if we return `null`. Is there a reason we try to handle it gracefully here?
IMO deferring the null-check to the users is more flexible. If we hardcode the check inside this function, things could become ugly if we add another user that wants the result of the lookup but doesn't want to panic if the lookup fails. 

We only have two callers here, so I guess the typing overhead is still acceptable?

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:180
@@ +179,3 @@
+
+  CFLGraph() = default;
+  CFLGraph(CFLGraph &&) = default;
----------------
george.burgess.iv wrote:
> FWIW, I had to remove these ctors from D21233, because they made the windows buildbots angry. Apparently it's a bug in MSVC2013.
> 
> If these don't have any use, please remove them from this patch, as well.
This will be fixed by the rebase.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:237
@@ +236,3 @@
+      Externals.insert(Val);
+    else if (auto CExpr = dyn_cast<ConstantExpr>(Val)) {
+      if (hasUsefulEdges(CExpr))
----------------
george.burgess.iv wrote:
> Style nit: Braces.
What exactly is the problem here? Should I remove the braces after "if (auto CExpr = ...", or should I add braces after "if (hasUsefulEdges..."?



================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:371
@@ -282,1 +370,3 @@
 
+  struct Edge {
+    // Which value the edge is coming from
----------------
george.burgess.iv wrote:
> Comment disappeared?
Ooops...

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:372
@@ +371,3 @@
+  struct Edge {
+    // Which value the edge is coming from
+    Value *From;
----------------
george.burgess.iv wrote:
> Is there a reason we went from `///` to `//` for these?
The struct becomes private. Only tryInterproceduralAnalysis() uses it now. 

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:520
@@ +519,3 @@
+    // anything, too (unless the result is marked noalias).
+    if (Targets.size() > 1 || (!CS.onlyReadsMemory())) {
+      for (Value *V : CS.args()) {
----------------
george.burgess.iv wrote:
> george.burgess.iv wrote:
> > Please remove the braces here and below...
> > 
> > (And the parens on `!CS.onlyReadsMemory()`)
> Why do we care about the size of `Targets` here? Looks like we'll mark nothing as escaped if we can't find a target of a function.
I wasn't quite sure whether to use CS.onlyReadsMemory() or CS.getCalledFunction()->onlyReadsMemory() here. It looks like the two functions essentially do the same thing. Initially used the latter and switched to the former after a while (the size check was there just to make sure CS.getCalledFunction() != nullptr). So I think I'll just remove it now.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:527
@@ +526,3 @@
+      auto Fn = CS.getCalledFunction();
+      if (Fn == nullptr || !Fn->doesNotAlias(0))
+        addEdge(Inst, Inst, EdgeType::Assign, AttrUnknown);
----------------
george.burgess.iv wrote:
> Looks like we won't add this to the sets if `Fn->doesNotAlias(0)`.
Which is intended: noalias return values are treated like mallocs.

================
Comment at: test/Analysis/CFLAliasAnalysis/attr-escape.ll:53
@@ +52,3 @@
+; CHECK: NoAlias: i32* %c, i32** %a
+define void @test_external_call_readonly(i32* %x) {
+	%a = alloca i32*, align 8
----------------
george.burgess.iv wrote:
> Can we have a test for callsites marked as `readonly`, (if we currently support them) as well?
I'm not aware that we can mark the callsite (instead of the function) readonly. How can we distinguish the two in our codes?


http://reviews.llvm.org/D21262