[PATCH] D53037: [InstCombine] combine a shuffle and an extract subvector shuffle

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D53037#1385795 <https://reviews.llvm.org/D53037#1385795>, @spatel wrote:

> In D53037#1385727 <https://reviews.llvm.org/D53037#1385727>, @efriedma wrote:
>
> > That case is specifically interesting because when it's split, each half is lowered to a different shuffle, and both shuffles are expensive.
> >
> > Obviously you could try to find that specific pattern in SelectionDAG on AArch64, but I'm not sure it makes sense to expect the backend to try; instcombine probably shouldn't be creating difficult-to-solve optimization puzzles.
>
>
> The goal for this patch was to make things easier for the backend, but I didn't think of this case of course. How about limiting the transform like this:
>
>   Index: InstCombine/InstCombineVectorOps.cpp
>   ===================================================================
>   --- InstCombine/InstCombineVectorOps.cpp	(revision 353169)
>   +++ InstCombine/InstCombineVectorOps.cpp	(working copy)
>   @@ -1498,6 +1498,11 @@
>      if (!match(Op0, m_ShuffleVector(m_Value(X), m_Value(Y), m_Constant(Mask))))
>        return nullptr;
>   
>   +  // Be extra conservative with shuffle transforms. If we can't kill the 1st
>   +  // shuffle, then combining may result in worse codegen.
>   +  if (!Op0->hasOneUse())
>   +    return nullptr;
>   +
>      // We are extracting a subvector from a shuffle. Remove excess elements from
>      // the 1st shuffle mask to eliminate the extract.
>      //
>  
>


That would solve the issue, thanks. I think it would make sense to keep this transform quite conservative, without knowing more about the backend. If this is helpful for some backends, it might make sense to do it at a later stage in the pipeline. Merging back shuffles in the backend might be possible in some cases, but I think it would be better if the backend does not have to fight instcombine in this case.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53037