[X86] Do not assume types are legal in getFauxShuffleMask

[bjope]

Summary

Make sure we do not assert on value types not being
simple in getFauxShuffleMask when analysing operations
such as "v8i16 = truncate v8i24".

[RKSimon]

llvm/lib/Target/X86/X86ISelLowering.cpp

7554–7558

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;

llvm/test/CodeGen/X86/shuffle-combine-crash-3.ll

61

This can probably be reduced/cleaned up more?

[bjope]

llvm/lib/Target/X86/X86ISelLowering.cpp

7554–7558

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.

llvm/test/CodeGen/X86/shuffle-combine-crash-3.ll

61

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.

[bjope]

Fixups after review. Managed to reduce the test case a bit.

[RKSimon]

LGTM - cheers

llvm/lib/Target/X86/X86ISelLowering.cpp

if (!IsUnary)
       Ops.push_back(N1);
 
     createPackShuffleMask(VT, Mask, IsUnary);
     return true;
   }
   case ISD::TRUNCATE:
   case X86ISD::VTRUNC: {
     SDValue Src = N.getOperand(0);
-    MVT SrcVT = Src.getSimpleValueType();
+    EVT SrcVT = Src.getValueType();
+    // Truncated source must be a simple vector.
+    if (!SrcVT.isSimple() || (SrcVT.getSizeInBits() % 128) != 0 ||
+        (SrcVT.getScalarSizeInBits() % 8) != 0)
+      return false;
     unsigned NumSrcElts = SrcVT.getVectorNumElements();
     unsigned NumBitsPerSrcElt = SrcVT.getScalarSizeInBits();
     unsigned Scale = NumBitsPerSrcElt / NumBitsPerElt;
     assert((NumBitsPerSrcElt % NumBitsPerElt) == 0 && "Illegal truncation");
     for (unsigned i = 0; i != NumSrcElts; ++i)
       Mask.push_back(i * Scale);
     Mask.append(NumElts - NumSrcElts, SM_SentinelZero);
     Ops.push_back(Src);

llvm/test/CodeGen/X86/shuffle-combine-crash-3.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown | FileCheck %s
+
+; Verify that we don't crash when compiling this. We used to hit an
+; assert like this
+;
+;   llc: ../include/llvm/CodeGen/ValueTypes.h:251: llvm::MVT llvm::EVT::getSimpleVT() const: Assertion `isSimple() && "Expected a SimpleValueType!"' failed.
+;
+; due to getFauxShuffleMask not checking that the VT was simple before a call
+; to getSimpleValueType().
+
+define i1 @dont_hit_assert(i24 signext %d) {
+; CHECK-LABEL: dont_hit_assert:
+; CHECK:       # %bb.0: # %for.cond
+; CHECK-NEXT:    movb $-1, %al
+; CHECK-NEXT:    negb %al
+; CHECK-NEXT:    sete %al
+; CHECK-NEXT:    retq
+for.cond:
+  %t0 = insertelement <8 x i24> zeroinitializer, i24 1, i32 0
+  %t5 = icmp slt <8 x i24> %t0, zeroinitializer
+  %t7 = icmp slt i24 0, %d
+  %rdx.shuf = shufflevector <8 x i1> %t5, <8 x i1> undef, <8 x i32> <i32 4, i32 5, i32 6, i32 7, i32 undef, i32 undef, i32 undef, i32 undef>
+  %bin.rdx = and <8 x i1> %t5, %rdx.shuf
+  %rdx.shuf22 = shufflevector <8 x i1> %bin.rdx, <8 x i1> undef, <8 x i32> <i32 2, i32 3, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef>
+  %bin.rdx23 = and <8 x i1> %bin.rdx, %rdx.shuf22
+  %rdx.shuf24 = shufflevector <8 x i1> %bin.rdx23, <8 x i1> undef, <8 x i32> <i32 1, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef>
+  %bin.rdx25 = and <8 x i1> %bin.rdx23, %rdx.shuf24
+  %t8 = extractelement <8 x i1> %bin.rdx25, i32 0
+  ret i1 %t8
+}