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

[Nicolai Hähnle via llvm-commits]

On 03.11.2016 14:52, Nirav Davé wrote:
> 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);
>   }

That makes sense to me, though I'm simplifying it to return directly out 
of the loop. Thank you for the review!

Nicolai

> On Thu, Nov 3, 2016 at 9:21 AM, Nicolai Hähnle <nhaehnle at gmail.com
> <mailto: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 <https://reviews.llvm.org/D25601>
>
>
>
>