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 ↗

(On Diff #185687)

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 ↗

(On Diff #185687)

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 ↗

(On Diff #185687)

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

4953 ↗

(On Diff #185687)

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 ↗

(On Diff #185687)

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

665–666 ↗

(On Diff #185687)

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

4953 ↗

(On Diff #185687)

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 ↗

(On Diff #185738)

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.

llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp

 }
 
 static Instruction *foldICmpBitCast(ICmpInst &Cmp,
                                     InstCombiner::BuilderTy &Builder) {
   auto *Bitcast = dyn_cast<BitCastInst>(Cmp.getOperand(0));
   if (!Bitcast)
     return nullptr;
 
-  // Zero-equality and sign-bit checks are preserved through sitofp + bitcast.
-  // FIXME: This needs to check that the bitcast does not change the number of
-  // elements in a vector.
   ICmpInst::Predicate Pred = Cmp.getPredicate();
   Value *Op1 = Cmp.getOperand(1);
   Value *BCSrcOp = Bitcast->getOperand(0);
+
+  // Make sure the bitcast doesn't change the number of vector elements.
+  if (Bitcast->getSrcTy()->getScalarSizeInBits() ==
+          Bitcast->getDestTy()->getScalarSizeInBits()) {
+    // Zero-equality and sign-bit checks are preserved through sitofp + bitcast.
     Value *X;
     if (match(BCSrcOp, 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()));
+        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(BCSrcOp, m_UIToFP(m_Value(X))))
       if (Cmp.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, do so.
   if (Bitcast->getType()->isPointerTy() &&
       (isa<Constant>(Op1) || isa<BitCastInst>(Op1))) {
     // If operand #1 is a bitcast instruction, it must also be a ptr->ptr cast
     // so eliminate it as well.
     if (auto *BC2 = dyn_cast<BitCastInst>(Op1))

llvm/trunk/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:    [[F:%.*]] = sitofp <3 x i16> [[I:%.*]] to <3 x float>
+; CHECK-NEXT:    [[B:%.*]] = bitcast <3 x float> [[F]] to <6 x i16>
+; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt <6 x i16> [[B]], <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:    [[F:%.*]] = sitofp <3 x i16> [[I:%.*]] to <3 x float>
+; CHECK-NEXT:    [[B:%.*]] = bitcast <3 x float> [[F]] to i96
+; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt i96 [[B]], -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:    [[F:%.*]] = uitofp <3 x i16> [[I:%.*]] to <3 x float>
+; CHECK-NEXT:    [[B:%.*]] = bitcast <3 x float> [[F]] to <6 x i16>
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq <6 x i16> [[B]], 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:    [[F:%.*]] = uitofp <3 x i16> [[I:%.*]] to <3 x float>
+; CHECK-NEXT:    [[B:%.*]] = bitcast <3 x float> [[F]] to i96
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i96 [[B]], 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
+}