[PATCH] D107009: [WIP][X86] combineX86ShuffleChain(): canonicalize mask elts picking from splats

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

A few comments



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:35827
+  SmallVector<int> NewMask;
+  if (V1IsSplat || V2IsSplat) {
+    NewMask.reserve(NumBaseMaskElts);
----------------
Both V1 and V2 could be smaller than RootSizeInBits - we need to either not try to do this in that case or bail in the loop below if we try to reference a 'identity' element higher than V1 or V2's width.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:35828
+  if (V1IsSplat || V2IsSplat) {
+    NewMask.reserve(NumBaseMaskElts);
+    for (unsigned i = 0; i != NumBaseMaskElts; ++i) {
----------------
Why not NewMask.assign(BaseMask.begin(), BaseMask.end()) - then we just need to adjust the masks instead of repeated emplace_back().


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:35831
+      int M = BaseMask[i];
+      assert((M < 0 || ((unsigned)M < 2 * NumBaseMaskElts)) && "OOB mask elt?");
+      int NewM = M;
----------------
Use isUndefOrInRange ?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:35844
+        }
+      }
+
----------------
This looks over-complicated? Merge some of the comments and avoid nested if()s


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:35848
+    }
+    BaseMask = NewMask;
+  }
----------------
BaseMask = std::move(NewMask);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107009