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

Tue Aug 3 12:55:33 PDT 2021

lebedev.ri added inline comments.

================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:35782-35783
   MVT VT2 = V2.getSimpleValueType();
   assert(VT1.getSizeInBits() == RootSizeInBits &&
          VT2.getSizeInBits() == RootSizeInBits && "Vector size mismatch");

----------------
RKSimon wrote:
> lebedev.ri wrote:
> > Didn't we just assert that the inputs have the same size as root?
> The middle-term plan is to relax these to (RootSizeInBit % VT1.getSizeInBits()) == 0 - we're getting pretty close and that will let us stop widening in combineX86ShufflesRecursively - which is causing hasOneUse() problems - and just widen when a match is found (which is what CanonicalizeShuffleInput will do). This should also help use get rid of combineX86ShuffleChainWithExtract.
> 
> But I can understand if you don't want to handle this yet.
Right. I'm aware that this is an artificial (and subpar) restriction.

================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:35827
+  SmallVector<int> NewMask;
+  if (V1IsSplat || V2IsSplat) {
+    NewMask.reserve(NumBaseMaskElts);
----------------
RKSimon wrote:
> 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.
Okay, does this look about right?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107009