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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

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.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:2299
+      X->getType()->isVectorTy() &&
+      cast<FixedVectorType>(X->getType())->getNumElements() == VWidth &&
+      X->getType() == Y->getType()) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97397