[PATCH] D97397: [InstCombine] Add a combine for a shuffle of identical bitcasts

[Sanne Wouda via Phabricator via llvm-commits]

sanwou01 added a comment.

In D97397#2585516 <https://reviews.llvm.org/D97397#2585516>, @spatel wrote:

> We intentionally do not create new shuffle masks in instcombine because we can't guarantee that codegen can lower arbitrary masks efficiently, but this patch seems fine since it just re-uses the existing mask.
> If there is motivation to handle casts of different-sized elements (and therefore requires a new mask), you might look at building on VectorCombine::foldBitcastShuf(). We use the cost model there to avoid creating unsupported shuffles.

I don't have a motivation to handle bitcasts that change the element size (or scalar to vector, for that matter), so I'd prefer to keep this simple.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:2299
+      X->getType()->isVectorTy() &&
+      cast<FixedVectorType>(X->getType())->getNumElements() == VWidth &&
+      X->getType() == Y->getType()) {
----------------
spatel wrote:
> lebedev.ri wrote:
> > sanwou01 wrote:
> > > lebedev.ri wrote:
> > > > Can we do anything for the case where we have element count mismatches?
> > > Yes, I'm being fairly conservative here. I think the following example would be legal to transform similarly. Is that what you had in mind?
> > > 
> > > ```
> > > %0 = bitcast <4 x i32> %a to <4 x float>
> > > %1 = bitcast <2 x i32> %b to <2 x float>
> > > %2 = shufflevector <4 x float> %0, <2 x float> %1, <2 x i32> <i32 3, i32 5>
> > > ```
> > I actually don't have anything particular in mind, just asking.
> > That being said, i think both operands of a `shufflevector` must have the same type
> > (including vector element count), so i'm not sure that example works?
> It should be ok to handle a length-changing shuffle, but we need to add type checks to make it safe.
> Note that as-is this patch doesn't have the right combination - this crashes:
> 
> ```
> define <4 x double> @bc_shuf(<4 x i32> %x, <4 x i32> %y) {
>   %xb = bitcast <4 x i32> %x to <2 x double>
>   %yb = bitcast <4 x i32> %y to <2 x double>
>   %r = shufflevector <2 x double> %xb, <2 x double> %yb, <4 x i32> <i32 1, i32 3, i32 0, i32 2>
>   ret <4 x double> %r
> }
> 
> ```
> 
> So definitely add some negative tests to make sure we don't break things.
Good catch, I've added tests for length-changing shuffles (which as you point out we can handle without changing the mask) and some negative tests for bitcasts that change the element size.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97397