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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:12665
 
+  auto canonicalizeBroadcastableInput =
+      [DL, &Subtarget, &DAG](SDValue &Input, MutableArrayRef<int> InputMask) {
----------------
At least a summary comment would be useful.


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


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


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:12673
+          return;
+        Input = getSplatOfVectorElement(DL, Input, 0, DAG);
+        for (auto I : enumerate(InputMask)) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108382