[InstSimplify][InstCombine] don't crash when folding vector selects of icmp

[spatel]

Summary

As Eli pointed out in D22537 , we have big chunks of duplicated code for similar folds in InstSimplify and InstCombine. I started on the path to refactoring that in rL276140, but it looks like it's going to be a long haul to make sure vectors work in all cases.

As Eli also noted in D22537, the code doesn't work for vector types. Not only does it not fold, it crashes. So this is hopefully a quick fix for both places before I get back to the refactoring job.

I think we need to keep the 'getTypeSizeInBits()' call to handle aggregate types, but use getScalarSizeInBits() for vectors.

Note that I've added a scalar test for the InstCombine fold because it doesn't exist anywhere. There are likely many more of these patterns that simply have no tests. I plan to continue adding those in subsequent patches.

[spatel]

Patch updated: we can use getScalarType() to make this cleaner.

[ibadawi]

Makes sense to me. I also applied your patch and verified that I was able to compile the original file that I reduced to get my reproducer.

Thanks for the quick turnaround on this.

[majnemer]

LGTM

[spatel]

For the record, it doesn't make any sense to get a sign bit for an aggregate, but I don't think we can ever reach a bug from that because we can't icmp an aggregate. I'll clean this up when it's rewritten.

lib/Analysis/InstructionSimplify.cpp

 static Value *simplifySelectWithICmpCond(Value *CondVal, Value *TrueVal,
                                          Value *FalseVal, const Query &Q,
                                          unsigned MaxRecurse) {
   ICmpInst::Predicate Pred;
   Value *CmpLHS, *CmpRHS;
   if (!match(CondVal, m_ICmp(Pred, m_Value(CmpLHS), m_Value(CmpRHS))))
     return nullptr;
 
-  unsigned BitWidth = Q.DL.getTypeSizeInBits(TrueVal->getType());
+  // FIXME: This code is nearly duplicated in InstCombine. Using/refactoring
+  // decomposeBitTestICmp() might help.
+  unsigned BitWidth = TrueVal->getType()->isVectorTy()
+                          ? TrueVal->getType()->getScalarSizeInBits()
+                          : Q.DL.getTypeSizeInBits(TrueVal->getType());
   APInt MinSignedValue = APInt::getSignBit(BitWidth);
   if (ICmpInst::isEquality(Pred) && match(CmpRHS, m_Zero())) {
     Value *X;
     const APInt *Y;
     if (match(CmpLHS, m_And(m_Value(X), m_APInt(Y))))
       if (Value *V = simplifySelectBitTest(TrueVal, FalseVal, X, Y,
                                            Pred == ICmpInst::ICMP_EQ))
         return V;

lib/Transforms/InstCombine/InstCombineSelect.cpp

if (CmpLHS == TrueVal && Pred == ICmpInst::ICMP_EQ) {
       Changed = true;
     } else if (CmpLHS == FalseVal && Pred == ICmpInst::ICMP_NE) {
       // Transform (X != C) ? Y : X -> (X != C) ? Y : C
       SI.setOperand(2, CmpRHS);
       Changed = true;
     }
   }
 
+  // FIXME: This code is nearly duplicated in InstSimplify. Using/refactoring
+  // decomposeBitTestICmp() might help.
   {
-    unsigned BitWidth = DL.getTypeSizeInBits(TrueVal->getType());
+    unsigned BitWidth = TrueVal->getType()->isVectorTy()
+                            ? TrueVal->getType()->getScalarSizeInBits()
+                            : DL.getTypeSizeInBits(TrueVal->getType());
     APInt MinSignedValue = APInt::getSignBit(BitWidth);
     Value *X;
     const APInt *Y, *C;
     bool TrueWhenUnset;
     bool IsBitTest = false;
     if (ICmpInst::isEquality(Pred) &&
         match(CmpLHS, m_And(m_Value(X), m_Power2(Y))) &&
         match(CmpRHS, m_Zero())) {

test/Transforms/InstCombine/select.ll

; CHECK-NEXT:  ret i32 %s1
 
   %not_a = xor i32 %a, -1
   %c0 = icmp slt i32 %a, 0
   %s0 = select i1 %c0, i32 %not_a, i32 -1
   %c1 = icmp sgt i32 %s0, -1
   %s1 = select i1 %c1, i32 %s0, i32 -1
   ret i32 %s1
 }
+
+define i32 @select_icmp_slt0_xor(i32 %x) {
+; CHECK-LABEL: @select_icmp_slt0_xor(
+; CHECK-NEXT:    [[TMP1:%.*]] = or i32 %x, -2147483648
+; CHECK-NEXT:    ret i32 [[TMP1]]
+;
+  %cmp = icmp slt i32 %x, zeroinitializer
+  %xor = xor i32 %x, 2147483648
+  %x.xor = select i1 %cmp, i32 %x, i32 %xor
+  ret i32 %x.xor
+}
+
+define <2 x i32> @select_icmp_slt0_xor_vec(<2 x i32> %x) {
+; CHECK-LABEL: @select_icmp_slt0_xor_vec(
+; CHECK-NEXT:    [[TMP1:%.*]] = or <2 x i32> %x, <i32 -2147483648, i32 -2147483648>
+; CHECK-NEXT:    ret <2 x i32> [[TMP1]]
+;
+  %cmp = icmp slt <2 x i32> %x, zeroinitializer
+  %xor = xor <2 x i32> %x, <i32 2147483648, i32 2147483648>
+  %x.xor = select <2 x i1> %cmp, <2 x i32> %x, <2 x i32> %xor
+  ret <2 x i32> %x.xor
+}
+

test/Transforms/InstSimplify/select.ll

 ; CHECK-NEXT:    ret i32 [[AND]]
 ;
   %cmp = icmp sgt i32 %X, -1
   %and = and i32 %X, 2147483647
   %cond = select i1 %cmp, i32 %X, i32 %and
   ret i32 %cond
 }
 
+define <2 x i8> @test11vec(<2 x i8> %X) {
+; CHECK-LABEL: @test11vec(
+; CHECK-NEXT:    [[AND:%.*]] = and <2 x i8> %X, <i8 127, i8 127>
+; CHECK-NEXT:    ret <2 x i8> [[AND]]
+;
+  %cmp = icmp sgt <2 x i8> %X, <i8 -1, i8 -1>
+  %and = and <2 x i8> %X, <i8 127, i8 127>
+  %sel = select <2 x i1> %cmp, <2 x i8> %X, <2 x i8> %and
+  ret <2 x i8> %sel
+}
+
 define i32 @select_icmp_and_8_eq_0_or_8(i32 %x) {
 ; CHECK-LABEL: @select_icmp_and_8_eq_0_or_8(
 ; CHECK-NEXT:    [[OR:%.*]] = or i32 %x, 8
 ; CHECK-NEXT:    ret i32 [[OR]]
 ;
   %and = and i32 %x, 8
   %cmp = icmp eq i32 %and, 0
   %or = or i32 %x, 8