[PATCH] D25601: DAGCombiner: fix use-after-free when merging consecutive stores

[Nirav Davé via llvm-commits]

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.

That said, it seems like we should replace EverChanged with STMerged which
I think is the actual condition we are checking.

    for (;;) {
      SmallVector<MemOpLink, 8> StoreNodes;
      bool Changed = MergeConsecutiveStores(ST, StoreNodes);
      if (!Changed) break;
      if (any_of(StoreNodes,
                 [ST](const MemOpLink &Link) { return Link.MemNode == ST;
}))
        STMerged = true;
    }

    if (STMerged)
      return SDValue(N, 0);
  }

On Thu, Nov 3, 2016 at 9:21 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:

> nhaehnle added a comment.
>
> 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.
>
> 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.
>
> Look, I'll do you one better and do a llvm-lit test/CodeGen run with the
> following change in a debug build:
>
>   if (any_of(StoreNodes,
>              [ST](const MemOpLink &Link) { return Link.MemNode == ST; })) {
>     assert(ST->getOpcode() == ISD::DELETED_NODE);
>     break;
>   }
>
>   assert(ST->getOpcode() != ISD::DELETED_NODE);
>
> 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.
>
>
> https://reviews.llvm.org/D25601
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161103/5677fbd0/attachment.html>