[InstCombine] Inefficient pattern for high-bits checking 2 (PR38708)

[lebedev.ri]

Summary

It is sometimes important to check that some newly-computed value
is non-negative and only n bits wide (where n is a variable.)
There are many ways to check that:
https://godbolt.org/z/o4RB8D
The last variant seems best?
(I'm sure there are some other variations i haven't thought of..)

More complicated, canonical pattern:
https://rise4fun.com/Alive/uhA

We do need to have two switch()'es like this,
to not mismatch the swappable predicates.

https://bugs.llvm.org/show_bug.cgi?id=38708

[spatel]

I was wondering if it would be easier to read if we swapped everything as a first step, so something like this in the existing code:

// We want X to be the icmp's second operand, so swap if not.
Value *Cmp0 = Cmp.getOperand(0), *X = Cmp.getOperand(1);
ICmpInst::Predicate Pred = Cmp.getPredicate(), NewPred;
if (match(X, m_OneUse(m_Shl(m_One(), m_Value())))) {
  std::swap(Cmp0, X);
  Pred = Cmp.getSwappedPredicate();
}
Value *Y;
if (!match(Cmp0, m_OneUse(m_Shl(m_One(), m_Value(Y)))))
  return nullptr;
if (Pred == ICmpInst::ICMP_ULE) NewPred = ICmpInst::ICMP_NE;
else if (Pred == ICmpInst::ICMP_UGT) NewPred = ICmpInst::ICMP_EQ;
else return nullptr;

It's mostly a matter of taste, but there's a subtle logic difference: what if both sides of the icmp match the shift pattern?

define i1 @p0(i8 %shamt0, i8 %shamt1) {
  %t0 = shl i8 1, %shamt0
  %t1 = shl i8 1, %shamt1
  %r = icmp ugt i8 %t0, %t1
  ret i1 %r
}

With the current code, it gets transformed to:

define i1 @p0(i8 %shamt0, i8 %shamt1) {
  %t1 = shl i8 1, %shamt1
  %t1.highbits = lshr i8 %t1, %shamt0
  %r = icmp eq i8 %t1.highbits, 0
  ret i1 %r
}

It should be reduced:

%t0 = shl i8 1, %shamt0
%t1 = shl i8 1, %shamt1
%r = icmp ugt i8 %t0, %t1
=>
%r = icmp ugt i8 %shamt0, %shamt1

I don't think we need to hold up this patch for that, but maybe it changes the way we want to implement it?

[lebedev.ri]

In D52001#1233768, @spatel wrote:

I was wondering if it would be easier to read if we swapped everything as a first step, so something like this in the existing code:

// We want X to be the icmp's second operand, so swap if not.
Value *Cmp0 = Cmp.getOperand(0), *X = Cmp.getOperand(1);
ICmpInst::Predicate Pred = Cmp.getPredicate(), NewPred;
if (match(X, m_OneUse(m_Shl(m_One(), m_Value())))) {
  std::swap(Cmp0, X);
  Pred = Cmp.getSwappedPredicate();
}
Value *Y;
if (!match(Cmp0, m_OneUse(m_Shl(m_One(), m_Value(Y)))))
  return nullptr;
if (Pred == ICmpInst::ICMP_ULE) NewPred = ICmpInst::ICMP_NE;
else if (Pred == ICmpInst::ICMP_UGT) NewPred = ICmpInst::ICMP_EQ;
else return nullptr;

I'm not sure.
I think we really should have two separate matches, and two switch()es.
Else i think we may use the wrong predicate..
Also, i *think* i will add one/two patterns to this new matcher (less canonical variants with extra uses),
so specifying the pattern twice seems sub-par.

It's mostly a matter of taste, but there's a subtle logic difference: what if both sides of the icmp match the shift pattern?

define i1 @p0(i8 %shamt0, i8 %shamt1) {
  %t0 = shl i8 1, %shamt0
  %t1 = shl i8 1, %shamt1
  %r = icmp ugt i8 %t0, %t1
  ret i1 %r
}

With the current code, it gets transformed to:

define i1 @p0(i8 %shamt0, i8 %shamt1) {
  %t1 = shl i8 1, %shamt1
  %t1.highbits = lshr i8 %t1, %shamt0
  %r = icmp eq i8 %t1.highbits, 0
  ret i1 %r
}

It should be reduced:

%t0 = shl i8 1, %shamt0
%t1 = shl i8 1, %shamt1
%r = icmp ugt i8 %t0, %t1
=>
%r = icmp ugt i8 %shamt0, %shamt1

I don't think we need to hold up this patch for that, but maybe it changes the way we want to implement it?

I acknowledge that there is some problem when we have the same/similar pattern on the both sides,
i have thought about it a bit (rL342076), but i don't have anything concrete on that.

[spatel]

In D52001#1233786, @lebedev.ri wrote:

In D52001#1233768, @spatel wrote:

I was wondering if it would be easier to read if we swapped everything as a first step, so something like this in the existing code:

// We want X to be the icmp's second operand, so swap if not.
Value *Cmp0 = Cmp.getOperand(0), *X = Cmp.getOperand(1);
ICmpInst::Predicate Pred = Cmp.getPredicate(), NewPred;
if (match(X, m_OneUse(m_Shl(m_One(), m_Value())))) {
  std::swap(Cmp0, X);
  Pred = Cmp.getSwappedPredicate();
}
Value *Y;
if (!match(Cmp0, m_OneUse(m_Shl(m_One(), m_Value(Y)))))
  return nullptr;
if (Pred == ICmpInst::ICMP_ULE) NewPred = ICmpInst::ICMP_NE;
else if (Pred == ICmpInst::ICMP_UGT) NewPred = ICmpInst::ICMP_EQ;
else return nullptr;

I'm not sure.
I think we really should have two separate matches, and two switch()es.
Else i think we may use the wrong predicate..
Also, i *think* i will add one/two patterns to this new matcher (less canonical variants with extra uses),
so specifying the pattern twice seems sub-par.

Ok - just wanted to throw it out as a possibility. I agree that the switch version is less likely to go buggy.

define i1 @p0(i8 %shamt0, i8 %shamt1) {
  %t0 = shl i8 1, %shamt0
  %t1 = shl i8 1, %shamt1
  %r = icmp ugt i8 %t0, %t1
  ret i1 %r
}

With the current code, it gets transformed to:

define i1 @p0(i8 %shamt0, i8 %shamt1) {
  %t1 = shl i8 1, %shamt1
  %t1.highbits = lshr i8 %t1, %shamt0
  %r = icmp eq i8 %t1.highbits, 0
  ret i1 %r
}

It should be reduced:

%t0 = shl i8 1, %shamt0
%t1 = shl i8 1, %shamt1
%r = icmp ugt i8 %t0, %t1
=>
%r = icmp ugt i8 %shamt0, %shamt1

I don't think we need to hold up this patch for that, but maybe it changes the way we want to implement it?

I acknowledge that there is some problem when we have the same/similar pattern on the both sides,
i have thought about it a bit (rL342076), but i don't have anything concrete on that.

Sounds good. Please add tests where both sides match, so we have some evidence of the missing folds. Mostly, I'm paranoid that we'll open up some infinite looping scenario if we don't have tests for those unexpected patterns. As we're finding with min/max, it's hard to see those problems in advance.

LGTM

[lebedev.ri]

In D52001#1233827, @spatel wrote:

In D52001#1233786, @lebedev.ri wrote:

In D52001#1233768, @spatel wrote:

I was wondering if it would be easier to read if we swapped everything as a first step, so something like this in the existing code:

// We want X to be the icmp's second operand, so swap if not.
Value *Cmp0 = Cmp.getOperand(0), *X = Cmp.getOperand(1);
ICmpInst::Predicate Pred = Cmp.getPredicate(), NewPred;
if (match(X, m_OneUse(m_Shl(m_One(), m_Value())))) {
  std::swap(Cmp0, X);
  Pred = Cmp.getSwappedPredicate();
}
Value *Y;
if (!match(Cmp0, m_OneUse(m_Shl(m_One(), m_Value(Y)))))
  return nullptr;
if (Pred == ICmpInst::ICMP_ULE) NewPred = ICmpInst::ICMP_NE;
else if (Pred == ICmpInst::ICMP_UGT) NewPred = ICmpInst::ICMP_EQ;
else return nullptr;

I'm not sure.
I think we really should have two separate matches, and two switch()es.
Else i think we may use the wrong predicate..
Also, i *think* i will add one/two patterns to this new matcher (less canonical variants with extra uses),
so specifying the pattern twice seems sub-par.

Ok - just wanted to throw it out as a possibility. I agree that the switch version is less likely to go buggy.

define i1 @p0(i8 %shamt0, i8 %shamt1) {
  %t0 = shl i8 1, %shamt0
  %t1 = shl i8 1, %shamt1
  %r = icmp ugt i8 %t0, %t1
  ret i1 %r
}

With the current code, it gets transformed to:

define i1 @p0(i8 %shamt0, i8 %shamt1) {
  %t1 = shl i8 1, %shamt1
  %t1.highbits = lshr i8 %t1, %shamt0
  %r = icmp eq i8 %t1.highbits, 0
  ret i1 %r
}

It should be reduced:

%t0 = shl i8 1, %shamt0
%t1 = shl i8 1, %shamt1
%r = icmp ugt i8 %t0, %t1
=>
%r = icmp ugt i8 %shamt0, %shamt1

I don't think we need to hold up this patch for that, but maybe it changes the way we want to implement it?

I acknowledge that there is some problem when we have the same/similar pattern on the both sides,
i have thought about it a bit (rL342076), but i don't have anything concrete on that.

Sounds good.

Please add tests where both sides match, so we have some evidence of the missing folds.

Will do.

Mostly, I'm paranoid that we'll open up some infinite looping scenario if we don't have tests for those unexpected patterns. As we're finding with min/max, it's hard to see those problems in advance.

LGTM

Thank you for the review!

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

case ICmpInst::ICMP_SGE:
     LLVM_FALLTHROUGH;
   case ICmpInst::ICMP_SLE:
     // icmp sle i1 A, B -> A | ~B
     return BinaryOperator::CreateOr(Builder.CreateNot(B), A);
   }
 }
 
 // Transform pattern like:
-//   (1 << Y) u<= X
-//   (1 << Y) u>  X
+//   (1 << Y) u<= X  or  ~(-1 << Y) u<  X
+//   (1 << Y) u>  X  or  ~(-1 << Y) u>= X
 // Into:
 //   (X l>> Y) != 0
 //   (X l>> Y) == 0
 static Instruction *foldICmpWithHighBitMask(ICmpInst &Cmp,
                                             InstCombiner::BuilderTy &Builder) {
-  ICmpInst::Predicate Pred;
+  ICmpInst::Predicate Pred, NewPred;
   Value *X, *Y;
-  if (!match(&Cmp,
-             m_c_ICmp(Pred, m_OneUse(m_Shl(m_One(), m_Value(Y))), m_Value(X))))
-    return nullptr;
-
-  // We want X to be the icmp's second operand, so swap predicate if it is not.
+  if (match(&Cmp,
+            m_c_ICmp(Pred, m_OneUse(m_Shl(m_One(), m_Value(Y))), m_Value(X)))) {
+    // We want X to be the icmp's second operand, so swap predicate if it isn't.
     if (Cmp.getOperand(0) == X)
       Pred = Cmp.getSwappedPredicate();
 
-  ICmpInst::Predicate NewPred;
     switch (Pred) {
     case ICmpInst::ICMP_ULE:
       NewPred = ICmpInst::ICMP_NE;
       break;
     case ICmpInst::ICMP_UGT:
       NewPred = ICmpInst::ICMP_EQ;
       break;
     default:
       return nullptr;
     }
+  } else if (match(&Cmp,
+                   m_c_ICmp(Pred,
+                            m_OneUse(m_Not(m_Shl(m_AllOnes(), m_Value(Y)))),
+                            m_Value(X)))) {
+    // We want X to be the icmp's second operand, so swap predicate if it isn't.
+    if (Cmp.getOperand(0) == X)
+      Pred = Cmp.getSwappedPredicate();
+
+    switch (Pred) {
+    case ICmpInst::ICMP_ULT:
+      NewPred = ICmpInst::ICMP_NE;
+      break;
+    case ICmpInst::ICMP_UGE:
+      NewPred = ICmpInst::ICMP_EQ;
+      break;
+    default:
+      return nullptr;
+    }
+  } else
+    return nullptr;
 
   Value *NewX = Builder.CreateLShr(X, Y, X->getName() + ".highbits");
   Constant *Zero = Constant::getNullValue(NewX->getType());
   return CmpInst::Create(Instruction::ICmp, NewPred, NewX, Zero);
 }
 
 static Instruction *foldVectorCmp(CmpInst &Cmp,
                                   InstCombiner::BuilderTy &Builder) {

llvm/trunk/test/Transforms/InstCombine/icmp-uge-of-not-of-shl-allones-by-bits-and-val-to-icmp-eq-of-lshr-val-by-bits-and-0.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -instcombine -S | FileCheck %s
 
 ; https://bugs.llvm.org/show_bug.cgi?id=38708
 
 ; Pattern:
 ;   ~(-1 << bits) u>= val
 ; Should be transformed into:
 ;   (val l>> bits) == 0
 
 ; ============================================================================ ;
 ; Basic positive tests
 ; ============================================================================ ;
 
 define i1 @p0(i8 %val, i8 %bits) {
 ; CHECK-LABEL: @p0(
-; CHECK-NEXT:    [[T0:%.*]] = shl i8 -1, [[BITS:%.*]]
-; CHECK-NEXT:    [[T1:%.*]] = xor i8 [[T0]], -1
-; CHECK-NEXT:    [[R:%.*]] = icmp uge i8 [[T1]], [[VAL:%.*]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr i8 [[VAL:%.*]], [[BITS:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[VAL_HIGHBITS]], 0
 ; CHECK-NEXT:    ret i1 [[R]]
 ;
   %t0 = shl i8 -1, %bits
   %t1 = xor i8 %t0, -1
   %r = icmp uge i8 %t1, %val
   ret i1 %r
 }
 
 ; ============================================================================ ;
 ; Vector tests
 ; ============================================================================ ;
 
 define <2 x i1> @p1_vec(<2 x i8> %val, <2 x i8> %bits) {
 ; CHECK-LABEL: @p1_vec(
-; CHECK-NEXT:    [[T0:%.*]] = shl <2 x i8> <i8 -1, i8 -1>, [[BITS:%.*]]
-; CHECK-NEXT:    [[T1:%.*]] = xor <2 x i8> [[T0]], <i8 -1, i8 -1>
-; CHECK-NEXT:    [[R:%.*]] = icmp uge <2 x i8> [[T1]], [[VAL:%.*]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr <2 x i8> [[VAL:%.*]], [[BITS:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq <2 x i8> [[VAL_HIGHBITS]], zeroinitializer
 ; CHECK-NEXT:    ret <2 x i1> [[R]]
 ;
   %t0 = shl <2 x i8> <i8 -1, i8 -1>, %bits
   %t1 = xor <2 x i8> %t0, <i8 -1, i8 -1>
   %r = icmp uge <2 x i8> %t1, %val
   ret <2 x i1> %r
 }
 
 define <3 x i1> @p2_vec_undef0(<3 x i8> %val, <3 x i8> %bits) {
 ; CHECK-LABEL: @p2_vec_undef0(
-; CHECK-NEXT:    [[T0:%.*]] = shl <3 x i8> <i8 -1, i8 undef, i8 -1>, [[BITS:%.*]]
-; CHECK-NEXT:    [[T1:%.*]] = xor <3 x i8> [[T0]], <i8 -1, i8 -1, i8 -1>
-; CHECK-NEXT:    [[R:%.*]] = icmp uge <3 x i8> [[T1]], [[VAL:%.*]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr <3 x i8> [[VAL:%.*]], [[BITS:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq <3 x i8> [[VAL_HIGHBITS]], zeroinitializer
 ; CHECK-NEXT:    ret <3 x i1> [[R]]
 ;
   %t0 = shl <3 x i8> <i8 -1, i8 undef, i8 -1>, %bits
   %t1 = xor <3 x i8> %t0, <i8 -1, i8 -1, i8 -1>
   %r = icmp uge <3 x i8> %t1, %val
   ret <3 x i1> %r
 }
 
 define <3 x i1> @p2_vec_undef1(<3 x i8> %val, <3 x i8> %bits) {
 ; CHECK-LABEL: @p2_vec_undef1(
-; CHECK-NEXT:    [[T0:%.*]] = shl <3 x i8> <i8 -1, i8 -1, i8 -1>, [[BITS:%.*]]
-; CHECK-NEXT:    [[T1:%.*]] = xor <3 x i8> [[T0]], <i8 -1, i8 undef, i8 -1>
-; CHECK-NEXT:    [[R:%.*]] = icmp uge <3 x i8> [[T1]], [[VAL:%.*]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr <3 x i8> [[VAL:%.*]], [[BITS:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq <3 x i8> [[VAL_HIGHBITS]], zeroinitializer
 ; CHECK-NEXT:    ret <3 x i1> [[R]]
 ;
   %t0 = shl <3 x i8> <i8 -1, i8 -1, i8 -1>, %bits
   %t1 = xor <3 x i8> %t0, <i8 -1, i8 undef, i8 -1>
   %r = icmp uge <3 x i8> %t1, %val
   ret <3 x i1> %r
 }
 
 define <3 x i1> @p2_vec_undef2(<3 x i8> %val, <3 x i8> %bits) {
 ; CHECK-LABEL: @p2_vec_undef2(
-; CHECK-NEXT:    [[T0:%.*]] = shl <3 x i8> <i8 -1, i8 undef, i8 -1>, [[BITS:%.*]]
-; CHECK-NEXT:    [[T1:%.*]] = xor <3 x i8> [[T0]], <i8 -1, i8 undef, i8 -1>
-; CHECK-NEXT:    [[R:%.*]] = icmp uge <3 x i8> [[T1]], [[VAL:%.*]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr <3 x i8> [[VAL:%.*]], [[BITS:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq <3 x i8> [[VAL_HIGHBITS]], zeroinitializer
 ; CHECK-NEXT:    ret <3 x i1> [[R]]
 ;
   %t0 = shl <3 x i8> <i8 -1, i8 undef, i8 -1>, %bits
   %t1 = xor <3 x i8> %t0, <i8 -1, i8 undef, i8 -1>
   %r = icmp uge <3 x i8> %t1, %val
   ret <3 x i1> %r
 }
 
 ; ============================================================================ ;
 ; Commutativity tests.
 ; ============================================================================ ;
 
 declare i8 @gen8()
 
 define i1 @c0(i8 %bits) {
 ; CHECK-LABEL: @c0(
-; CHECK-NEXT:    [[T0:%.*]] = shl i8 -1, [[BITS:%.*]]
-; CHECK-NEXT:    [[T1:%.*]] = xor i8 [[T0]], -1
 ; CHECK-NEXT:    [[VAL:%.*]] = call i8 @gen8()
-; CHECK-NEXT:    [[R:%.*]] = icmp ule i8 [[VAL]], [[T1]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr i8 [[VAL]], [[BITS:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[VAL_HIGHBITS]], 0
 ; CHECK-NEXT:    ret i1 [[R]]
 ;
   %t0 = shl i8 -1, %bits
   %t1 = xor i8 %t0, -1
   %val = call i8 @gen8()
   %r = icmp ule i8 %val, %t1 ; swapped order and predicate
   ret i1 %r
 }

 ; ============================================================================ ;
 
 declare void @use8(i8)
 
 define i1 @oneuse0(i8 %val, i8 %bits) {
 ; CHECK-LABEL: @oneuse0(
 ; CHECK-NEXT:    [[T0:%.*]] = shl i8 -1, [[BITS:%.*]]
 ; CHECK-NEXT:    call void @use8(i8 [[T0]])
-; CHECK-NEXT:    [[T1:%.*]] = xor i8 [[T0]], -1
-; CHECK-NEXT:    [[R:%.*]] = icmp uge i8 [[T1]], [[VAL:%.*]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr i8 [[VAL:%.*]], [[BITS]]
+; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[VAL_HIGHBITS]], 0
 ; CHECK-NEXT:    ret i1 [[R]]
 ;
   %t0 = shl i8 -1, %bits
   call void @use8(i8 %t0)
   %t1 = xor i8 %t0, -1
   %r = icmp uge i8 %t1, %val
   ret i1 %r
 }

llvm/trunk/test/Transforms/InstCombine/icmp-ult-of-not-of-shl-allones-by-bits-and-val-to-icmp-ne-of-lshr-val-by-bits-and-0.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -instcombine -S | FileCheck %s
 
 ; https://bugs.llvm.org/show_bug.cgi?id=38708
 
 ; Pattern:
 ;   ~(-1 << bits) u< val
 ; Should be transformed into:
 ;   (val l>> bits) != 0
 
 ; ============================================================================ ;
 ; Basic positive tests
 ; ============================================================================ ;
 
 define i1 @p0(i8 %val, i8 %bits) {
 ; CHECK-LABEL: @p0(
-; CHECK-NEXT:    [[T0:%.*]] = shl i8 -1, [[BITS:%.*]]
-; CHECK-NEXT:    [[T1:%.*]] = xor i8 [[T0]], -1
-; CHECK-NEXT:    [[R:%.*]] = icmp ult i8 [[T1]], [[VAL:%.*]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr i8 [[VAL:%.*]], [[BITS:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = icmp ne i8 [[VAL_HIGHBITS]], 0
 ; CHECK-NEXT:    ret i1 [[R]]
 ;
   %t0 = shl i8 -1, %bits
   %t1 = xor i8 %t0, -1
   %r = icmp ult i8 %t1, %val
   ret i1 %r
 }
 
 ; ============================================================================ ;
 ; Vector tests
 ; ============================================================================ ;
 
 define <2 x i1> @p1_vec(<2 x i8> %val, <2 x i8> %bits) {
 ; CHECK-LABEL: @p1_vec(
-; CHECK-NEXT:    [[T0:%.*]] = shl <2 x i8> <i8 -1, i8 -1>, [[BITS:%.*]]
-; CHECK-NEXT:    [[T1:%.*]] = xor <2 x i8> [[T0]], <i8 -1, i8 -1>
-; CHECK-NEXT:    [[R:%.*]] = icmp ult <2 x i8> [[T1]], [[VAL:%.*]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr <2 x i8> [[VAL:%.*]], [[BITS:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = icmp ne <2 x i8> [[VAL_HIGHBITS]], zeroinitializer
 ; CHECK-NEXT:    ret <2 x i1> [[R]]
 ;
   %t0 = shl <2 x i8> <i8 -1, i8 -1>, %bits
   %t1 = xor <2 x i8> %t0, <i8 -1, i8 -1>
   %r = icmp ult <2 x i8> %t1, %val
   ret <2 x i1> %r
 }
 
 define <3 x i1> @p2_vec_undef0(<3 x i8> %val, <3 x i8> %bits) {
 ; CHECK-LABEL: @p2_vec_undef0(
-; CHECK-NEXT:    [[T0:%.*]] = shl <3 x i8> <i8 -1, i8 undef, i8 -1>, [[BITS:%.*]]
-; CHECK-NEXT:    [[T1:%.*]] = xor <3 x i8> [[T0]], <i8 -1, i8 -1, i8 -1>
-; CHECK-NEXT:    [[R:%.*]] = icmp ult <3 x i8> [[T1]], [[VAL:%.*]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr <3 x i8> [[VAL:%.*]], [[BITS:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = icmp ne <3 x i8> [[VAL_HIGHBITS]], zeroinitializer
 ; CHECK-NEXT:    ret <3 x i1> [[R]]
 ;
   %t0 = shl <3 x i8> <i8 -1, i8 undef, i8 -1>, %bits
   %t1 = xor <3 x i8> %t0, <i8 -1, i8 -1, i8 -1>
   %r = icmp ult <3 x i8> %t1, %val
   ret <3 x i1> %r
 }
 
 define <3 x i1> @p2_vec_undef1(<3 x i8> %val, <3 x i8> %bits) {
 ; CHECK-LABEL: @p2_vec_undef1(
-; CHECK-NEXT:    [[T0:%.*]] = shl <3 x i8> <i8 -1, i8 -1, i8 -1>, [[BITS:%.*]]
-; CHECK-NEXT:    [[T1:%.*]] = xor <3 x i8> [[T0]], <i8 -1, i8 undef, i8 -1>
-; CHECK-NEXT:    [[R:%.*]] = icmp ult <3 x i8> [[T1]], [[VAL:%.*]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr <3 x i8> [[VAL:%.*]], [[BITS:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = icmp ne <3 x i8> [[VAL_HIGHBITS]], zeroinitializer
 ; CHECK-NEXT:    ret <3 x i1> [[R]]
 ;
   %t0 = shl <3 x i8> <i8 -1, i8 -1, i8 -1>, %bits
   %t1 = xor <3 x i8> %t0, <i8 -1, i8 undef, i8 -1>
   %r = icmp ult <3 x i8> %t1, %val
   ret <3 x i1> %r
 }
 
 define <3 x i1> @p2_vec_undef2(<3 x i8> %val, <3 x i8> %bits) {
 ; CHECK-LABEL: @p2_vec_undef2(
-; CHECK-NEXT:    [[T0:%.*]] = shl <3 x i8> <i8 -1, i8 undef, i8 -1>, [[BITS:%.*]]
-; CHECK-NEXT:    [[T1:%.*]] = xor <3 x i8> [[T0]], <i8 -1, i8 undef, i8 -1>
-; CHECK-NEXT:    [[R:%.*]] = icmp ult <3 x i8> [[T1]], [[VAL:%.*]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr <3 x i8> [[VAL:%.*]], [[BITS:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = icmp ne <3 x i8> [[VAL_HIGHBITS]], zeroinitializer
 ; CHECK-NEXT:    ret <3 x i1> [[R]]
 ;
   %t0 = shl <3 x i8> <i8 -1, i8 undef, i8 -1>, %bits
   %t1 = xor <3 x i8> %t0, <i8 -1, i8 undef, i8 -1>
   %r = icmp ult <3 x i8> %t1, %val
   ret <3 x i1> %r
 }
 
 ; ============================================================================ ;
 ; Commutativity tests.
 ; ============================================================================ ;
 
 declare i8 @gen8()
 
 define i1 @c0(i8 %bits) {
 ; CHECK-LABEL: @c0(
-; CHECK-NEXT:    [[T0:%.*]] = shl i8 -1, [[BITS:%.*]]
-; CHECK-NEXT:    [[T1:%.*]] = xor i8 [[T0]], -1
 ; CHECK-NEXT:    [[VAL:%.*]] = call i8 @gen8()
-; CHECK-NEXT:    [[R:%.*]] = icmp ugt i8 [[VAL]], [[T1]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr i8 [[VAL]], [[BITS:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = icmp ne i8 [[VAL_HIGHBITS]], 0
 ; CHECK-NEXT:    ret i1 [[R]]
 ;
   %t0 = shl i8 -1, %bits
   %t1 = xor i8 %t0, -1
   %val = call i8 @gen8()
   %r = icmp ugt i8 %val, %t1 ; swapped order and predicate
   ret i1 %r
 }

 ; ============================================================================ ;
 
 declare void @use8(i8)
 
 define i1 @oneuse0(i8 %val, i8 %bits) {
 ; CHECK-LABEL: @oneuse0(
 ; CHECK-NEXT:    [[T0:%.*]] = shl i8 -1, [[BITS:%.*]]
 ; CHECK-NEXT:    call void @use8(i8 [[T0]])
-; CHECK-NEXT:    [[T1:%.*]] = xor i8 [[T0]], -1
-; CHECK-NEXT:    [[R:%.*]] = icmp ult i8 [[T1]], [[VAL:%.*]]
+; CHECK-NEXT:    [[VAL_HIGHBITS:%.*]] = lshr i8 [[VAL:%.*]], [[BITS]]
+; CHECK-NEXT:    [[R:%.*]] = icmp ne i8 [[VAL_HIGHBITS]], 0
 ; CHECK-NEXT:    ret i1 [[R]]
 ;
   %t0 = shl i8 -1, %bits
   call void @use8(i8 %t0)
   %t1 = xor i8 %t0, -1
   %r = icmp ult i8 %t1, %val
   ret i1 %r
 }