[PATCH] D18336: Prevent construction of cycle in DAG store merge

[James Y Knight via llvm-commits]

jyknight added inline comments.

================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:628
@@ -627,13 +627,3 @@
 
-  /// Return true if N is a predecessor of this node.
-  /// N is either an operand of this node, or can be reached by recursively
-  /// traversing up the operands.
-  /// In this helper the Visited and worklist sets are held externally to
-  /// cache predecessors over multiple invocations. If you want to test for
-  /// multiple predecessors this method is preferable to multiple calls to
-  /// hasPredecessor. Be sure to clear Visited and Worklist if the DAG
-  /// changes.
-  /// NOTE: This is still very expensive. Use carefully.
-  bool hasPredecessorHelper(const SDNode *N,
-                            SmallPtrSetImpl<const SDNode *> &Visited,
-                            SmallVectorImpl<const SDNode *> &Worklist) const;
+  /// Populate Visitedo Set as unino of operand_dependences of nodes in
+  /// Worklist. Stop short if N is found (Use Null if complete closure is
----------------
Lots of typos. Visitedo. unino. "operand_dependences" sounds like it's referring to a variable or function but isn't actually.

"Null" is strangely capitalized.

Much of the old comment you deleted seems still relevant, too.

================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:634
@@ +633,3 @@
+                                   SmallVectorImpl<const SDNode *> &Worklist) {
+    while (!Worklist.empty()) {
+      const SDNode *M = Worklist.pop_back_val();
----------------
Why removed the Visited check? It seems to me like that was still a more natural interface.

Then you can just ask all your "is Node X a predecessor" questions with the same interface.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9638
@@ -9638,1 +9637,3 @@
+  Worklist.push_back(N);
+  SDNode::hasPredecessorHelper(NULL, Visited, Worklist); // search completely
 
----------------
Why search completely? Isn't it better to escape early if you happen to be able to?

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11158-11160
@@ -11156,1 +11157,5 @@
 
+  // Any pair of candidates here may cause a problem, but as all
+  // stores are on parallel chains so in the correct case, none of
+  // the nodes will be predecessors of any other. Check in parallel
+  SmallPtrSet<const SDNode *, 16> Visited;
----------------
This comment isn't very clear.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11166
@@ +11165,3 @@
+    SDNode *n = StoreNodes[i].MemNode;
+    for (unsigned j = 1; j < n->getNumOperands(); ++j)
+      Worklist.push_back(n->getOperand(j).getNode());
----------------
That you're skipping the chain operand deserves a mention, here.

================
Comment at: test/CodeGen/AArch64/vector_merge_dep_check.ll:12
@@ +11,3 @@
+entry:
+  br i1 undef, label %polly.loop_header134, label %polly.cond184
+
----------------
Yes, I see that. It's just that there's a lot of (probably?) irrelevant metadata args which can be removed, variable names shortened/made clearer, possibly the loop could go away too.

And a comment in the test about what it is actually intending to test would be good, too.


http://reviews.llvm.org/D18336