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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D70223#1759762 <https://reviews.llvm.org/D70223#1759762>, @qiucf wrote:

> 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?


I don't understand how checking uses would change that. Do you have an example?

The code comment is still accurate in general because instcombine must be good for all targets and not all targets have a generic shuffle instruction - Altivec vperm is a real luxury. :)
So it's very difficult to reverse shuffle transforms later. As an example, see how much code x86 needs to map shuffles to a series of incomplete shuffle ISAs under combineShuffle():
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/X86/X86ISelLowering.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70223