[PATCH] D77136: [X86] Do not assume types are legal in getFauxShuffleMask

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope marked 2 inline comments as done.
bjope added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:7528
+      return false;
     MVT SrcVT = Src.getSimpleValueType();
     unsigned NumSrcElts = SrcVT.getVectorNumElements();
----------------
RKSimon wrote:
> Nice catch! We actually need to be more thorough, matching what we do for the extensions (further down the function):
> ```
> SDValue Src = N.getOperand(0);
> EVT SrcVT = Src.getValueType();
> 
> // Truncated source must be a simple vector.
> if (!SrcVT.isSimple() || (SrcVT.getSizeInBits() % 128) != 0 ||
>     (SrcVT.getScalarSizeInBits() % 8) != 0)
>   return false;
> ```
Ok, I'll copy that.

I was wondering if perhaps this whole thing should be avoided until we have legalized types? But maybe we want to do these combines earlier than that for some reason. But then I guess we still need the tests on bit sizes, so it wouldn't be enough.


================
Comment at: llvm/test/CodeGen/X86/shuffle-combine-crash-3.ll:60
+  %9 = and i1 %8, %7
+  br i1 %9, label %for.cond.9, label %if.then.i
+
----------------
RKSimon wrote:
> This can probably be reduced/cleaned up more?
I did some attempts to reduce it (for example removing the first insertelement/shufflevector instructions using input arguments, but then I got a lot more asm instructions in the output).

But given the feedback that the code change is reasonable, then I feel that I can invest some more time on reducing the test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77136