[PATCH] D70223: [DAGCombine] Split vector load-update-store into single element stores

[Qiu Chaofan via Phabricator via llvm-commits]

qiucf added a comment.

In D70223#1755719 <https://reviews.llvm.org/D70223#1755719>, @efriedma wrote:

> It looks like this is missing some checks on the load.  The code needs to check that the load and store target the same address, and that there aren't any operations between the load and the store that could modify the memory.
>
> The profitability check probably needs to weigh the cost of the memory operations a little more carefully in cases where the total number of memory operations increases.
>
> I'm a little worried there could be a performance penalty on certain CPUs if the vector value is loaded soon afterwards, due to the partial overlap.  Depends on details of the specific CPU, though, and maybe it's rare enough that it doesn't matter.


This patch might be split into two: (1) Merge `shuffle-insert` and `shuffle-shuffle` if they have no other uses and RHS in `shuffle` is constant; (2) Make `MatchVectorStoreSplit` only consider a simple `load-shuffle/insert-store` chain.

That may exclude cases that both LHS and RHS are results of `shuffle` but from the same root. However the case should be rare and complex.

My question is: why we didn't implement merging `shuffle`s in instcombine? I saw comments <https://github.com/llvm/llvm-project/blob/a71c1e2a576a6b0f85cab2bef12446b0e3967853/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp#L2039-L2081> saying that's unsafe or making things worse:

> we are absolutely afraid of producing a shuffle mask not in the input program, because the code gen may not be smart enough to turn a merged shuffle into two specific shuffles: it may produce worse code.  As such, we only merge two shuffles if the result is either a splat or one of the input shuffle masks.  In this case, merging the shuffles just removes one instruction, which we know is safe.

Would it help if we check their uses before fold them?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70223/new/

https://reviews.llvm.org/D70223