[PATCH] D108382: [X86] lowerShuffleAsDecomposedShuffleMerge(): if both inputs are broadcastable/identities, canonicalize broadcasts as such

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:12666
+  auto canonicalizeBroadcastableInput =
+      [DL, &Subtarget, &DAG](SDValue &Input, MutableArrayRef<int> InputMask) {
+        unsigned EltSizeInBits = Input.getScalarValueSizeInBits();
----------------
RKSimon wrote:
> Instead of passing Input by reference - why not return it? It just makes it look messy imo.
Note that we also modify `InputMask` - we turn it into an identity mask.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:12672
+        if (isNoopShuffleMask(InputMask) || !isBroadcastShuffleMask(InputMask))
+          return;
+        Input = getSplatOfVectorElement(DL, Input, 0, DAG);
----------------
RKSimon wrote:
> Would it be better to assert(isBroadcastShuffleMask(InputMask)) ? The isNoopOrBroadcastShuffleMask checks below should ensure it no?
Sure, that can work now.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:12673
+          return;
+        Input = getSplatOfVectorElement(DL, Input, 0, DAG);
+        for (auto I : enumerate(InputMask)) {
----------------
RKSimon wrote:
> Why not just create a X86ISD::VBROADCAST node? This code is AVX only and we have isel patterns that handle AVX1 cases where load folding fails.
Oh, hmm. I have not considered that, and that changes the results somewhat...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108382