<div dir="ltr"><div>It does reduce merges in the context of D14834 which was my worry. It's not clear if it's preferable to store merges as far as we can or give up immediately, but perhaps that's better folded into D14834 anyways. Given your explanation that's an issue with the SDNode recycler and not some other issue in the DAGCombiner, my concerns are put to rest.</div><div><br></div><div>That said, it seems like we should replace EverChanged with STMerged which I think is the actual condition we are checking. </div><div><div><div><div><br></div><div>    for (;;) {</div><div>      SmallVector<MemOpLink, 8> StoreNodes;<br></div><div>      bool Changed = MergeConsecutiveStores(ST, StoreNodes);</div><div>      if (!Changed) break;<br></div><div>      if (any_of(StoreNodes,<br></div><div>                 [ST](const MemOpLink &Link) { return Link.MemNode == ST; }))</div><div>        STMerged = true;</div><div>    }</div><div><br></div><div>    if (STMerged)</div><div>      return SDValue(N, 0);</div><div>  }</div></div></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 3, 2016 at 9:21 AM, Nicolai Hähnle <span dir="ltr"><<a href="mailto:nhaehnle@gmail.com" target="_blank">nhaehnle@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">nhaehnle added a comment.<br>
<br>
This doesn't reduce the number of merges. The patch has 0 changes in any of the test/CodeGen tests across all backends, and I don't have any out-of-tree tests with changes either. There is no real functional change.<br>
<br>
Even before this patch, the code satisfied: The store nodes that are deleted for the merge are exactly a prefix of the StoreNodes vector. The patch (a) truncates the StoreNodes vector to that prefix and (b) passes the vector out so that we can safely decide whether ST was deleted without implicitly relying on the behaviour of the SDNode recycler.<br>
<br>
Look, I'll do you one better and do a llvm-lit test/CodeGen run with the following change in a debug build:<br>
<br>
  if (any_of(StoreNodes,<br>
             [ST](const MemOpLink &Link) { return Link.MemNode == ST; })) {<br>
    assert(ST->getOpcode() == ISD::DELETED_NODE);<br>
    break;<br>
  }<br>
<br>
  assert(ST->getOpcode() != ISD::DELETED_NODE);<br>
<br>
Of course the assertions re-introduce the problematic reliance on the recycler behaviour, but the point is that neither assertion ever triggers on any of the in-tree CodeGen tests.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D25601" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D25601</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>