Fix some cases where icmp (bitcast ([su]itofp X)), Y is misfolded

[AndrewScheidecker]

Summary

This fixes a class of bugs introduced by https://reviews.llvm.org/D44367, which transforms various cases of icmp (bitcast ([su]itofp X)), Y to icmp X, Y. If the bitcast is between vector types with a different number of elements, the current code will produce bad IR along the lines of: icmp <N x i32> ..., <M x i32> <...>.

This patch suppresses the transform if the bitcast changes the number of vector elements.

[lebedev.ri]

test/Transforms/InstCombine/cast-int-icmp-eq-0.ll

665–666

I agree that this currently crashes, but are you sure this is valid in the first place?
%f = sitofp <3 x i16> %i to <3 x float> implies that sizeof(float) == sizeof(i16)
%b = bitcast <3 x float> %f to <6 x i16> implies that sizeof(float) == 2*sizeof(i16).
How can bitcast do that?

https://llvm.org/docs/LangRef.html#bitcast-to-instruction

The bit sizes of value and the destination type, ty2, must be identical.
It is always a no-op cast because no bits change with this conversion.
The conversion is done as if the value had been stored to memory and read back as type ty2.

[craig.topper]

test/Transforms/InstCombine/cast-int-icmp-eq-0.ll

665–666

sizeof(float) is always 32 bits. Why does sitofp <3 x i16> %i to <3 x float> imply sizeof(float) == sizeof(i16)? There's no requirement that the integer and fp width be equal for that.

[lebedev.ri]

lib/Transforms/InstCombine/InstCombineCompares.cpp

4827–4828

clang-format would have not put { on a new line.

4952–4953

Adjust comment too?
// Zero-equality and sign-bit checks are preserved through sitofp + bitcast, if bitcast does not change the number of elements.

test/Transforms/InstCombine/cast-int-icmp-eq-0.ll

658

Can you please also duplicated these cases with bitcast casting fp vector to an scalar integer?

665–666

sizeof(float) is always 32 bits.

Aha. Haven't fully woken up yet.
Which means the bitcast is correct, and sitofp is "increasing the bit width".

[AndrewScheidecker]

Addressed comments

[lebedev.ri]

OK, looks good, but might be a good idea for someone else to look.
While this bug was already released in 7.0, it is still a bug,
so you might want to file backport request for 8.0

lib/Transforms/InstCombine/InstCombineCompares.cpp

4952–4953

Hmm, the comment i suggested reads wrong/alien, but i can't suggest a better one.

[spatel]

Since we're capturing the bitcast below this block anyway, we could just move these transforms into that block as an NFC preliminary commit. Then, you can check that # of elts hasn't changed via a 1-line patch:

if (CI->getSrcTy()->getScalarSizeInBits() ==
       CI->getDestTy()->getScalarSizeInBits()) {

Draft patch of the combined changes:

Index: lib/Transforms/InstCombine/InstCombineCompares.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineCompares.cpp	(revision 353405)
+++ lib/Transforms/InstCombine/InstCombineCompares.cpp	(working copy)
@@ -4945,34 +4945,6 @@
         return New;
   }
 
-  // Zero-equality and sign-bit checks are preserved through sitofp + bitcast.
-  Value *X;
-  if (match(Op0, m_BitCast(m_SIToFP(m_Value(X))))) {
-    // icmp  eq (bitcast (sitofp X)), 0 --> icmp  eq X, 0
-    // icmp  ne (bitcast (sitofp X)), 0 --> icmp  ne X, 0
-    // icmp slt (bitcast (sitofp X)), 0 --> icmp slt X, 0
-    // icmp sgt (bitcast (sitofp X)), 0 --> icmp sgt X, 0
-    if ((Pred == ICmpInst::ICMP_EQ || Pred == ICmpInst::ICMP_SLT ||
-         Pred == ICmpInst::ICMP_NE || Pred == ICmpInst::ICMP_SGT) &&
-        match(Op1, m_Zero()))
-      return new ICmpInst(Pred, X, ConstantInt::getNullValue(X->getType()));
-
-    // icmp slt (bitcast (sitofp X)), 1 --> icmp slt X, 1
-    if (Pred == ICmpInst::ICMP_SLT && match(Op1, m_One()))
-      return new ICmpInst(Pred, X, ConstantInt::get(X->getType(), 1));
-
-    // icmp sgt (bitcast (sitofp X)), -1 --> icmp sgt X, -1
-    if (Pred == ICmpInst::ICMP_SGT && match(Op1, m_AllOnes()))
-      return new ICmpInst(Pred, X, ConstantInt::getAllOnesValue(X->getType()));
-  }
-
-  // Zero-equality checks are preserved through unsigned floating-point casts:
-  // icmp eq (bitcast (uitofp X)), 0 --> icmp eq X, 0
-  // icmp ne (bitcast (uitofp X)), 0 --> icmp ne X, 0
-  if (match(Op0, m_BitCast(m_UIToFP(m_Value(X)))))
-    if (I.isEquality() && match(Op1, m_Zero()))
-      return new ICmpInst(Pred, X, ConstantInt::getNullValue(X->getType()));
-
   // Test to see if the operands of the icmp are casted versions of other
   // values.  If the ptr->ptr cast can be stripped off both arguments, we do so
   // now.
@@ -4999,6 +4971,38 @@
       }
       return new ICmpInst(I.getPredicate(), Op0, Op1);
     }
+    // Make sure we have the same number of elements through the bitcast
+    if (CI->getSrcTy()->getScalarSizeInBits() ==
+           CI->getDestTy()->getScalarSizeInBits()) {
+      // Zero-equality and sign-bit checks are preserved through sitofp+bitcast.
+      Value *X;
+      if (match(CI->getOperand(0), m_SIToFP(m_Value(X)))) {
+        // icmp  eq (bitcast (sitofp X)), 0 --> icmp  eq X, 0
+        // icmp  ne (bitcast (sitofp X)), 0 --> icmp  ne X, 0
+        // icmp slt (bitcast (sitofp X)), 0 --> icmp slt X, 0
+        // icmp sgt (bitcast (sitofp X)), 0 --> icmp sgt X, 0
+        if ((Pred == ICmpInst::ICMP_EQ || Pred == ICmpInst::ICMP_SLT ||
+             Pred == ICmpInst::ICMP_NE || Pred == ICmpInst::ICMP_SGT) &&
+            match(Op1, m_Zero()))
+          return new ICmpInst(Pred, X, ConstantInt::getNullValue(X->getType()));
+
+        // icmp slt (bitcast (sitofp X)), 1 --> icmp slt X, 1
+        if (Pred == ICmpInst::ICMP_SLT && match(Op1, m_One()))
+          return new ICmpInst(Pred, X, ConstantInt::get(X->getType(), 1));
+
+        // icmp sgt (bitcast (sitofp X)), -1 --> icmp sgt X, -1
+        if (Pred == ICmpInst::ICMP_SGT && match(Op1, m_AllOnes()))
+          return new ICmpInst(Pred, X,
+                              ConstantInt::getAllOnesValue(X->getType()));
+      }
+
+      // Zero-equality checks are preserved through unsigned FP casts:
+      // icmp eq (bitcast (uitofp X)), 0 --> icmp eq X, 0
+      // icmp ne (bitcast (uitofp X)), 0 --> icmp ne X, 0
+      if (match(CI->getOperand(0), m_UIToFP(m_Value(X))))
+        if (I.isEquality() && match(Op1, m_Zero()))
+          return new ICmpInst(Pred, X, ConstantInt::getNullValue(X->getType()));
+    }
   }
 
   if (isa<CastInst>(Op0)) {

lib/Transforms/InstCombine/InstCombineCompares.cpp

4827

Nit: it seems in-between to have this be a separate but local helper. I'd make it a lambda closer to the users or make it a helper inside of 'class Type' so it can be used everywhere.

[AndrewScheidecker]

Incorporated @spatel's proposed changes

[spatel]

LGTM - sorry about the bug!

[AndrewScheidecker]

No problem.

Can somebody commit this for me?

[spatel]

In D57871#1389411, @AndrewScheidecker wrote:

No problem.

Can somebody commit this for me?

Sure, I'll do that.

lib/Transforms/InstCombine/InstCombineCompares.cpp

if (match(LHS, m_ShuffleVector(m_Value(V1), m_Undef(), m_Constant(M))) &&
     CmpInst::Predicate P = Cmp.getPredicate();
     Value *NewCmp = isa<ICmpInst>(Cmp) ? Builder.CreateICmp(P, V1, V2)
                                        : Builder.CreateFCmp(P, V1, V2);
     return new ShuffleVectorInst(NewCmp, UndefValue::get(NewCmp->getType()), M);
   }
   return nullptr;
 }
 
+static unsigned getVectorNumElementsOrZero(Type *T) {
+  return T->isVectorTy() ? T->getVectorNumElements() : 0;
+}
+
 Instruction *InstCombiner::visitICmpInst(ICmpInst &I) {
   bool Changed = false;
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
   unsigned Op0Cplxity = getComplexity(Op0);
   unsigned Op1Cplxity = getComplexity(Op1);
 
   /// Orders the operands of the compare so that they are listed from most
   /// complex to least complex.  This puts constants before unary operators,
if (Op0->getType()->isPointerTy() && I.isEquality()) {
     if (auto *Alloca = dyn_cast<AllocaInst>(GetUnderlyingObject(Op0, DL)))
       if (Instruction *New = foldAllocaCmp(I, Alloca, Op1))
         return New;
     if (auto *Alloca = dyn_cast<AllocaInst>(GetUnderlyingObject(Op1, DL)))
       if (Instruction *New = foldAllocaCmp(I, Alloca, Op0))
         return New;
   }
 
-  // Zero-equality and sign-bit checks are preserved through sitofp + bitcast.
+  // Zero-equality and sign-bit checks are preserved through sitofp + bitcast,
+  // if bitcast does not change the number of elements.
   Value *X;
-  if (match(Op0, m_BitCast(m_SIToFP(m_Value(X))))) {
+  if (match(Op0, m_BitCast(m_SIToFP(m_Value(X)))) &&
+      getVectorNumElementsOrZero(X->getType()) ==
+          getVectorNumElementsOrZero(I.getType())) {
     // icmp  eq (bitcast (sitofp X)), 0 --> icmp  eq X, 0
     // icmp  ne (bitcast (sitofp X)), 0 --> icmp  ne X, 0
     // icmp slt (bitcast (sitofp X)), 0 --> icmp slt X, 0
     // icmp sgt (bitcast (sitofp X)), 0 --> icmp sgt X, 0
     if ((Pred == ICmpInst::ICMP_EQ || Pred == ICmpInst::ICMP_SLT ||
          Pred == ICmpInst::ICMP_NE || Pred == ICmpInst::ICMP_SGT) &&
         match(Op1, m_Zero()))
       return new ICmpInst(Pred, X, ConstantInt::getNullValue(X->getType()));
 
     // icmp slt (bitcast (sitofp X)), 1 --> icmp slt X, 1
     if (Pred == ICmpInst::ICMP_SLT && match(Op1, m_One()))
       return new ICmpInst(Pred, X, ConstantInt::get(X->getType(), 1));
 
     // icmp sgt (bitcast (sitofp X)), -1 --> icmp sgt X, -1
     if (Pred == ICmpInst::ICMP_SGT && match(Op1, m_AllOnes()))
       return new ICmpInst(Pred, X, ConstantInt::getAllOnesValue(X->getType()));
   }
 
   // Zero-equality checks are preserved through unsigned floating-point casts:
   // icmp eq (bitcast (uitofp X)), 0 --> icmp eq X, 0
   // icmp ne (bitcast (uitofp X)), 0 --> icmp ne X, 0
   if (match(Op0, m_BitCast(m_UIToFP(m_Value(X)))))
+    if (getVectorNumElementsOrZero(X->getType()) ==
+        getVectorNumElementsOrZero(I.getType()))
       if (I.isEquality() && match(Op1, m_Zero()))
         return new ICmpInst(Pred, X, ConstantInt::getNullValue(X->getType()));
 
   // Test to see if the operands of the icmp are casted versions of other
   // values.  If the ptr->ptr cast can be stripped off both arguments, we do so
   // now.
   if (BitCastInst *CI = dyn_cast<BitCastInst>(Op0)) {
     if (Op0->getType()->isPointerTy() &&
         (isa<Constant>(Op1) || isa<BitCastInst>(Op1))) {
       // We keep moving the cast from the left operand over to the right

test/Transforms/InstCombine/cast-int-icmp-eq-0.ll

 ; CHECK-NEXT:    ret <3 x i1> [[CMP]]
 ;
   %f = sitofp <3 x i16> %i to  <3 x float>
   %b = bitcast <3 x float> %f to <3 x i32>
   %cmp = icmp sgt <3 x i32> %b, <i32 -1, i32 undef, i32 -1>
   ret <3 x i1> %cmp
 }
 
+; Verify that the various forms of this transform are not applied when the
+; bitcast changes the number of vector elements:
+;   icmp (bitcast ([su]itofp X)), Y -> icmp X, Y
+
+define <6 x i1> @i16_cast_cmp_sgt_int_m1_bitcast_vector_num_elements_sitofp(<3 x i16> %i) {
+; CHECK-LABEL: @i16_cast_cmp_sgt_int_m1_bitcast_vector_num_elements_sitofp(
+; CHECK-NEXT:    [[SITOFP:%.*]] = sitofp <3 x i16> [[I:%.*]] to  <3 x float>
+; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast <3 x float> [[SITOFP]] to <6 x i16>
+; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt <6 x i16> [[BITCAST]], <i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1>
+; CHECK-NEXT:    ret <6 x i1> [[CMP]]
+;
+  %f = sitofp <3 x i16> %i to  <3 x float>
+  %b = bitcast <3 x float> %f to <6 x i16>
+  %cmp = icmp sgt <6 x i16> %b, <i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1>
+  ret <6 x i1> %cmp
+}
+
+define i1 @i16_cast_cmp_sgt_int_m1_bitcast_vector_to_scalar_sitofp(<3 x i16> %i) {
+; CHECK-LABEL: @i16_cast_cmp_sgt_int_m1_bitcast_vector_to_scalar_sitofp(
+; CHECK-NEXT:    [[SITOFP:%.*]] = sitofp <3 x i16> [[I:%.*]] to  <3 x float>
+; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast <3 x float> [[SITOFP]] to i96
+; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt i96 [[BITCAST]], -1
+; CHECK-NEXT:    ret i1 [[CMP]]
+;
+  %f = sitofp <3 x i16> %i to  <3 x float>
+  %b = bitcast <3 x float> %f to i96
+  %cmp = icmp sgt i96 %b, -1
+  ret i1 %cmp
+}
+
+
+define <6 x i1> @i16_cast_cmp_eq_int_0_bitcast_vector_num_elements_uitofp(<3 x i16> %i) {
+; CHECK-LABEL: @i16_cast_cmp_eq_int_0_bitcast_vector_num_elements_uitofp(
+; CHECK-NEXT:    [[UITOFP:%.*]] = uitofp <3 x i16> [[I:%.*]] to  <3 x float>
+; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast <3 x float> [[UITOFP]] to <6 x i16>
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq <6 x i16> [[BITCAST]], zeroinitializer
+; CHECK-NEXT:    ret <6 x i1> [[CMP]]
+;
+  %f = uitofp <3 x i16> %i to <3 x float>
+  %b = bitcast <3 x float> %f to <6 x i16>
+  %cmp = icmp eq <6 x i16> %b, <i16 0, i16 0, i16 0, i16 0, i16 0, i16 0>
+  ret <6 x i1> %cmp
+}
+
+define i1 @i16_cast_cmp_eq_int_0_bitcast_vector_to_scalar_uitofp(<3 x i16> %i) {
+; CHECK-LABEL: @i16_cast_cmp_eq_int_0_bitcast_vector_to_scalar_uitofp(
+; CHECK-NEXT:    [[UITOFP:%.*]] = uitofp <3 x i16> [[I:%.*]] to  <3 x float>
+; CHECK-NEXT:    [[BITCAST:%.*]] = bitcast <3 x float> [[UITOFP]] to i96
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i96 [[BITCAST]], 0
+; CHECK-NEXT:    ret i1 [[CMP]]
+;
+  %f = uitofp <3 x i16> %i to <3 x float>
+  %b = bitcast <3 x float> %f to i96
+  %cmp = icmp eq i96 %b, 0
+  ret i1 %cmp
+}
+