[PATCH] D123801: [DAG] Combine shuffle(bitcast(X), Mask) to bitcast(shuffle(X, Mask'))

Tue Apr 19 06:29:58 PDT 2022

dmgreen added a comment.

In D123801#3454381 <https://reviews.llvm.org/D123801#3454381>, @spatel wrote:

> This looks like a partial inverse of VectorCombine::foldBitcastShuf(). Would it help to limit that transform instead of or in addition to this?

This is coming from the expansion of concats as a DAG combine (see 1ba8f4f67dcf52 <https://reviews.llvm.org/rG1ba8f4f67dcf52cf628caa6e84c3526e936fa6b4>) but this seemed to help in general. In that insert-extend.ll case there are `shuffle(bitcast(buildvectors` involved, but it sounds beneficial to canonicalize towards the larger type if the shuffles are otherwise equivalent.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:21968
+  }
+
+  // Create the new shuffle with the new mask and bitcast it bask to the
----------------
RKSimon wrote:
> We need a TLI.isShuffleMaskLegal check?
I can add one. I'm pretty sure that isShuffleMaskLegal really means "is this shuffle cheap", with the way it is used at the moment. We can always convert a shuffle back (providing we haven't made anything less undef), so it might not really need to be cheap.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:22381
   // back to their original types.
   if (N0.getOpcode() == ISD::BITCAST && N0.hasOneUse() &&
       N1.isUndef() && Level < AfterLegalizeVectorOps &&
----------------
RKSimon wrote:
> We already have this - maybe we'd be better off adding support for binary bitcasted shuffles here?
Do you mean move the combineShuffleOfBitcast call here? I can do that.
This looks like it's performing a `shuffle(bitcast(shuffle(..` combine.

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

https://reviews.llvm.org/D123801