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

[James Y Knight via llvm-commits]

jyknight added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11136
@@ +11135,3 @@
+
+static void markDependence(SDNode* N) {
+  if (!N)
----------------
I'm not sure if it's valid to use setNodeId here. But that's irrelevant, because this function shouldn't exist, since the code should be using hasPredecessor.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11182
@@ +11181,3 @@
+
+  //Check if merging store candidate would cause a loop.
+  if (UseAA){
----------------
There are formatting/spacing issues throughout this. Please clang-format.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11192
@@ +11191,3 @@
+      Worklist.push_back(StoreNodes[i].MemNode);
+    (DAG.getEntryNode().getNode())->
+        hasPredecessorHelper(StoreNodes[0].MemNode, Visited, Worklist);
----------------
This is obfuscated; please refactor the hasPredecessor family to have a static function which requires the worklist to be pre-populated, instead of passing in a dummy node here.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11205
@@ +11204,3 @@
+      Node.setNodeId(Unmarked);
+    markDependence(LatestOp);
+    for (unsigned i=0; i < NumStores; ++i) {
----------------
Should use hasPredecessor.

================
Comment at: test/CodeGen/AArch64/vector_merge_dep_check.ll:10
@@ +9,3 @@
+; Function Attrs: noinline norecurse nounwind ssp uwtable
+define void @_ZN5Eigen8internal13gemm_pack_rhsINSt3__17complexIfEElLi2ELi0ELb0ELb1EEclEPS4_PKS4_lllll() #0 align 2 {
+entry:
----------------
This test can almost certainly be simplified. It would be nice to spend a bit of time to remove irrelevant stuff from it so it's easier to see what's actually going on is just:
 %l1 = load
 store half of %l1 -> offset 0
 %l2 = load
 store half of %l2 -> offset -1
 store half of %l2 -> offset 1

And then, merging the store to offsets -1 and 0 into a single store, which results in the %l2 load chain-depending on it, but yet the merged-store requires that load as a value.

(I think)


http://reviews.llvm.org/D18336