This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by AndrewScheidecker on Feb 6 2019, 6:18 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 6:18 PM
lebedev.ri added inline comments.Feb 6 2019, 11:35 PM
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 added inline comments.
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 added inline comments.Feb 7 2019, 12:04 AM
lib/Transforms/InstCombine/InstCombineCompares.cpp
4827–4828

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

4947–4948

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".

Addressed comments

AndrewScheidecker marked 3 inline comments as done.Feb 7 2019, 3:58 AM
lebedev.ri accepted this revision.Feb 7 2019, 5:08 AM

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
4947–4948

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

This revision is now accepted and ready to land.Feb 7 2019, 5:08 AM
spatel added a comment.Feb 7 2019, 8:23 AM

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.

Incorporated @spatel's proposed changes

spatel accepted this revision.Feb 7 2019, 11:17 AM

LGTM - sorry about the bug!

No problem.

Can somebody commit this for me?

No problem.

Can somebody commit this for me?

Sure, I'll do that.

This revision was automatically updated to reflect the committed changes.