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

[t.p.northover]

Summary

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.

[t.p.northover]

Fixing capitalization of variable. Oops.

[niravd]

LGTM.

[t.p.northover]

Thanks. Committed as r345200.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

   // Search Ops of store candidates.
   for (unsigned i = 0; i < NumStores; ++i) {
     SDNode *N = StoreNodes[i].MemNode;
     // Of the 4 Store Operands:
     //   * Chain (Op 0) -> We have already considered these
     //                    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)
     if (SDNode::hasPredecessorHelper(StoreNodes[i].MemNode, Visited, Worklist,
                                      Max))
       return false;
   return true;
 }