[PATCH] D53658: [DAG] Inspect more store operands for cycle before merging.

[Tim Northover via Phabricator via llvm-commits]

t.p.northover created this revision.
t.p.northover added a reviewer: niravd.
Herald added subscribers: hiraditya, kristof.beyls, javed.absar, mcrosier.

When checking whether merging some stores would create a Cycle we need to look at more of the operands. The address operands are all semantically related (because they must be contiguous), but don't necessarily refer to the same actual base SDNodes, which leaves room for a cycle to form (we saw this happen via an indexed load). I also believe the indexed amount operand (which is usually just undef) could participate in a loop since it can be non-constant on 32-bit ARM-mode ARM; I don't have even an internal test-case where that happens though.

So this extends the check to cover those operands too.

Unfortunately there's no test-case. I had a reasonably simple one (two stores and two memsets, with associated GEPs), but it was so fragile it didn't even survive the move from our internal branch to OSS LLVM (i.e. ToT doesn't trigger it). I can't see a way around that. Hopefully the updated comments will discourage anyone from regressing it.


Repository:
  rL LLVM

https://reviews.llvm.org/D53658

Files:
  llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp


Index: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
===================================================================
--- llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -14316,14 +14316,14 @@
     //                    in candidate selection and can be
     //                    safely ignored
     //   * Value (Op 1) -> Cycles may happen (e.g. through load chains)
-    //   * Address (Op 2) -> Merged addresses may only vary by a fixed constant
-    //                      and so no cycles are possible.
-    //   * (Op 3) -> appears to always be undef. Cannot be source of cycle.
-    //
-    // Thus we need only check predecessors of the value operands.
-    auto *Op = N->getOperand(1).getNode();
-    if (Visited.insert(Op).second)
-      Worklist.push_back(Op);
+    //   * Address (Op 2) -> Merged addresses may only vary by a fixed constant,
+    //                       but aren't necessarily fromt the same base node, so
+    //                       cycles possible (e.g. via indexed store).
+    //   * (Op 3) -> Represents the pre or post-indexing offset (or undef for
+    //               non-indexed stores). Not constant on all targets (e.g. ARM)
+    //               and so can participate in a cycle.
+    for (unsigned j = 1; j < n->getNumOperands(); ++j)
+      Worklist.push_back(n->getOperand(j).getNode());
   }
   // Search through DAG. We can stop early if we find a store node.
   for (unsigned i = 0; i < NumStores; ++i)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D53658.170924.patch
Type: text/x-patch
Size: 1511 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181024/fcba06b0/attachment.bin>