[PATCH] D29845: [SelectionDAG] Remove redundant stores more aggressively.

[Nirav Dave via Phabricator via llvm-commits]

niravd added a comment.

Hmm. This doesn't look like it is correct as it is currently upstream much less with this change. I'm not sure if we can get a problematic graph wtihout useAA on, so perhaps this is why we never noticed before. The chain dependence subgraph does _not_ capture the full dependencies of any two nodes in the subgraph. It only captures straight control dependencies in frame. This is important for efficiency and allows stack-level memory operations to be elided if unused.

This underlying transformation in visitSTORE effectively tries to reduce the parallelism in the DAG by pulling the relevant memory operation in the parallel TokenFactor group to the end. This could be done with calls of predecessorHelper though it's possible too expensive. If this is encoded in the graph it should be safe to do, but as it is currently it looks like multiple applications of the dead store elimination might be invalid.  I haven't thought deeply enough about the extract vector case, but there's a possibility of incorrect behavior with respect multiple threads peeking at each other's stack.

In https://reviews.llvm.org/D29845#680306, @efriedma wrote:

> Ugh, studied this a bit more, this is wrong.
>
> The problem is that TokenFactor isn't guaranteed to be the minimal set chain values.  Consider something like this:
>
> chain1, val1 = load entrytoken...
>  chain2 = store chain1...
>  chain3 = tokenfactor chain1, chain2
>  chain4 = store chain3, val1, ...
>
> The presence of chain1 in the tokenfactor is irrelevant; the only correct ordering is chain1, chain2, chain3, chain4, so chain2 might clobber the load.
>
> I think I can come up with a more targeted fix which is correct.





Repository:
  rL LLVM

https://reviews.llvm.org/D29845