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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

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)) {



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:4827
 
+static unsigned getVectorNumElementsOrZero(Type *T) {
+  return T->isVectorTy() ? T->getVectorNumElements() : 0;
----------------
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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57871/new/

https://reviews.llvm.org/D57871