Remove assert from ShuffleVectorInst

[samparker]

Summary

Which triggers on valid, but not useful, IR such as a undef mask.

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=46276

[uabelho]

Thanks for the patch so quickly!
I really have no idea if the assert actually makes sense or not though, so it's best if @spatel can have an opinion.

[spatel]

Was the failure only a fuzzer problem?
Ideally, callers shouldn't be wasting their time on analysis of degenerate code, so that assert might have actually been useful for real code.
Also, if this did happen in real code, we probably don't want to claim that this shuffle uses a source value if the shuffle doesn't actually exist (see inline comment).

llvm/lib/IR/Instructions.cpp

2056

I think it would be better to change to:

// Allow for degenerate case: completely undef mask means neither source is used.
return UsesLHS || UsesRHS;

llvm/test/CodeGen/X86/cgp_shuffle_crash.ll

1 ↗

(On Diff #270051)

I prefer a more direct test of CGP using "opt -codegenprepare -S".
That test should live in this directory:
llvm/test/Transforms/CodeGenPrepare/X86

Also, the test can be reduced to a single block (not sure if this is minimal):

define <2 x i8> @autogen_SD20565(i32 %x) {
  %Shuf = shufflevector <2 x i8> zeroinitializer, <2 x i8> zeroinitializer, <2 x i32> undef
  %Cmp = icmp slt i32 480483, %x
  %B = mul <2 x i8> %Shuf, %Shuf
  %S = select i1 %Cmp, <2 x i8> %B, <2 x i8> zeroinitializer
  ret <2 x i8> %Shuf
}

[samparker]

Was the failure only a fuzzer problem?

Not sure, but either it seems harsh to assert of technically legal IR.

Ideally, callers shouldn't be wasting their time on analysis of degenerate code, so that assert might have actually been useful for real code.

I'll have a look into the callsites to see if it's easier enough to skip on undef masks.

llvm/lib/IR/Instructions.cpp

2056

Looks good.

[samparker]

• Changed return value and added comment.
• Added a few internal UndefValue where possible.
• Moved test.

[spatel]

LGTM

llvm/lib/IR/Instructions.cpp

2056

Formatting: 80-column.

llvm/test/Transforms/CodeGenPrepare/X86/cgp_shuffle_crash.ll

14

bugpoint tends to produce tests like this with undef/unused values, and then those tests wiggle or become worthless when unrelated improvements are made. For that reason, I still prefer my more-defined version of the test. :)

[samparker]

llvm/test/Transforms/CodeGenPrepare/X86/cgp_shuffle_crash.ll

14

Ah, sorry, will do.

[uabelho]

In D81634#2087298, @spatel wrote:

Was the failure only a fuzzer problem?

My case showed up when generating llc input with llvm-stress.

llvm/lib/IR/Instructions.cpp

if (Mask[i] == -1)
       continue;
     assert(Mask[i] >= 0 && Mask[i] < (NumOpElts * 2) &&
            "Out-of-bounds shuffle mask element");
     UsesLHS |= (Mask[i] < NumOpElts);
     UsesRHS |= (Mask[i] >= NumOpElts);
     if (UsesLHS && UsesRHS)
       return false;
   }
-  assert((UsesLHS ^ UsesRHS) && "Should have selected from exactly 1 source");
-  return true;
+  // Allow for degenerate case: completely undef mask means neither source is used.
+  return UsesLHS || UsesRHS;
 }
 
 bool ShuffleVectorInst::isSingleSourceMask(ArrayRef<int> Mask) {
   // We don't have vector operand size information, so assume operands are the
   // same size as the mask.
   return isSingleSourceMaskImpl(Mask, Mask.size());
 }
 
bool ShuffleVectorInst::isExtractSubvectorMask(ArrayRef<int> Mask,
   if (0 <= SubIndex && SubIndex + (int)Mask.size() <= NumSrcElts) {
     Index = SubIndex;
     return true;
   }
   return false;
 }
 
 bool ShuffleVectorInst::isIdentityWithPadding() const {
+  if (isa<UndefValue>(Op<2>()))
+    return false;
   int NumOpElts = cast<VectorType>(Op<0>()->getType())->getNumElements();
   int NumMaskElts = cast<VectorType>(getType())->getNumElements();
   if (NumMaskElts <= NumOpElts)
     return false;
 
   // The first part of the mask must choose elements from exactly 1 source op.
   ArrayRef<int> Mask = getShuffleMask();
   if (!isIdentityMaskImpl(Mask, NumOpElts))
     return false;
 
   // All extending must be with undef elements.
   for (int i = NumOpElts; i < NumMaskElts; ++i)
     if (Mask[i] != -1)
       return false;
 
   return true;
 }
 
 bool ShuffleVectorInst::isIdentityWithExtract() const {
+  if (isa<UndefValue>(Op<2>()))
+    return false;
   int NumOpElts = cast<VectorType>(Op<0>()->getType())->getNumElements();
   int NumMaskElts = getType()->getNumElements();
   if (NumMaskElts >= NumOpElts)
     return false;
 
   return isIdentityMaskImpl(getShuffleMask(), NumOpElts);
 }
 
 bool ShuffleVectorInst::isConcat() const {
   // Vector concatenation is differentiated from identity with padding.
-  if (isa<UndefValue>(Op<0>()) || isa<UndefValue>(Op<1>()))
+  if (isa<UndefValue>(Op<0>()) || isa<UndefValue>(Op<1>()) ||
+      isa<UndefValue>(Op<2>()))
     return false;
 
   int NumOpElts = cast<VectorType>(Op<0>()->getType())->getNumElements();
   int NumMaskElts = getType()->getNumElements();
   if (NumMaskElts != NumOpElts * 2)
     return false;
 
   // Use the mask length rather than the operands' vector lengths here. We

llvm/test/Transforms/CodeGenPrepare/X86/cgp_shuffle_crash.ll

+; RUN: opt -codegenprepare -S %s | FileCheck %s
+
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK-LABEL: shuffle_one_source
+
+define <2 x i8> @shuffle_one_source(i32 %x) {
+  %Shuf = shufflevector <2 x i8> zeroinitializer, <2 x i8> zeroinitializer, <2 x i32> undef
+  %Cmp = icmp slt i32 480483, %x
+  %B = mul <2 x i8> %Shuf, %Shuf
+  %S = select i1 %Cmp, <2 x i8> %B, <2 x i8> zeroinitializer
+  ret <2 x i8> %Shuf
+}
+