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

[George Burgess IV via llvm-commits]

george.burgess.iv added inline comments.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:132
@@ +131,3 @@
+class CFLGraph {
+private:
+  typedef Value *Node;
----------------
Not done

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:158-212
@@ -153,4 +157,57 @@
 
-  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);
+    if (Itr == NodeImpls.end())
+      return nullptr;
+    return &Itr->second;
+  }
+  EdgeList *getNode(Node N) {
+    auto Itr = NodeImpls.find(N);
+    if (Itr == NodeImpls.end())
+      return nullptr;
+    return &Itr->second;
+  }
+
+  static Node nodeDeref(const NodeMap::value_type &P) { return P.first; }
+  typedef std::pointer_to_unary_function<const NodeMap::value_type &, Node>
+      NodeDerefFun;
+
+public:
+  typedef EdgeList::const_iterator const_edge_iterator;
+  typedef mapped_iterator<NodeMap::const_iterator, NodeDerefFun>
+      const_node_iterator;
+
+  CFLGraph() = default;
+  CFLGraph(CFLGraph &&) = default;
+  CFLGraph &operator=(CFLGraph &&) = default;
+
+  bool addNode(Node N) {
+    return NodeImpls.insert(std::make_pair(N, EdgeList())).second;
+  }
+
+  void addEdge(Node From, Node To, EdgeType Type,
+               StratifiedAttrs Attr = StratifiedAttrs()) {
+    auto FromEdges = getNode(From);
+    assert(FromEdges != nullptr);
+    auto ToEdges = getNode(To);
+    assert(ToEdges != nullptr);
+
+    FromEdges->push_back(Edge{Attr, Type, To});
+    ToEdges->push_back(Edge{Attr, flipWeight(Type), From});
+  }
+
+  iterator_range<const_edge_iterator> edgesFor(Node N) const {
+    auto Edges = getNode(N);
+    assert(Edges != nullptr);
+    return make_range(Edges->begin(), Edges->end());
+  }
+
+  iterator_range<const_node_iterator> nodes() const {
+    return make_range<const_node_iterator>(
+        map_iterator(NodeImpls.begin(), NodeDerefFun(nodeDeref)),
+        map_iterator(NodeImpls.end(), NodeDerefFun(nodeDeref)));
+  }
+
+  bool empty() const { return NodeImpls.empty(); }
+  std::size_t size() const { return NodeImpls.size(); }
 };
----------------
If we see the flexibility being useful in the future, then I'm fine with it; I was just wondering if this was intentional or not.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:180
@@ +179,3 @@
+
+  CFLGraph() = default;
+  CFLGraph(CFLGraph &&) = default;
----------------
Looks like they might have slipped by. :)

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:237
@@ +236,3 @@
+      Externals.insert(Val);
+    else if (auto CExpr = dyn_cast<ConstantExpr>(Val)) {
+      if (hasUsefulEdges(CExpr))
----------------
The former, please.

Generally, the style rule is "useless braces shouldn't exist, unless they help readability somehow."

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:372
@@ +371,3 @@
+  // Encodes the notion of a "use"
+  struct Edge {
+    // Which value the edge is coming from
----------------
I don't see what that has to do with the comment style, but I agree that it probably doesn't matter.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:517
@@ +516,3 @@
+    // readonly or readnone), and that the result could alias just about
+    // anything, too (unless the result is marked noalias).
+    if (!CS.onlyReadsMemory())
----------------
Sounds good.

(Also, it looks like  `CS.onlyReadsMemory()` will check for callsite attributes (e.g. readonly/readnone) for us, so you picked the better option there, I think.)

================
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);
----------------
grievejia wrote:
> 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.
Ah, I forgot that we added the nodes at the beginning of the function now -- my mistake. :)

================
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
----------------
grievejia wrote:
> 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?
If you're talking about tests, `call void @foo(args) readonly` is the syntax, IIRC. It's definitely less common than just using a function-level attribute.

If you're talking about in CFLAA, I think I sort of addressed that in an earlier response.


http://reviews.llvm.org/D21262