[llvm-commits] [llvm] r123203 - in /llvm/trunk: lib/Target/README.txt lib/Transforms/InstCombine/InstCombineCompares.cpp test/Transforms/InstCombine/icmp.ll

[Nick Lewycky]

Owen Anderson wrote:
> Author: resistor
> Date: Mon Jan 10 18:36:45 2011
> New Revision: 123203
>
> URL: http://llvm.org/viewvc/llvm-project?rev=123203&view=rev
> Log:
> Fix a random missed optimization by making InstCombine more aggressive when determining which bits are demanded by
> a comparison against a constant.
>
> Modified:
>      llvm/trunk/lib/Target/README.txt
>      llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>      llvm/trunk/test/Transforms/InstCombine/icmp.ll
>
> Modified: llvm/trunk/lib/Target/README.txt
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/README.txt?rev=123203&r1=123202&r2=123203&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/README.txt (original)
> +++ llvm/trunk/lib/Target/README.txt Mon Jan 10 18:36:45 2011
> @@ -1627,21 +1627,6 @@
>
>   //===---------------------------------------------------------------------===//
>
> -InstCombine should use SimplifyDemandedBits to remove the or instruction:
> -
> -define i1 @test(i8 %x, i8 %y) {
> -  %A = or i8 %x, 1
> -  %B = icmp ugt i8 %A, 3
> -  ret i1 %B
> -}
> -
> -Currently instcombine calls SimplifyDemandedBits with either all bits or just
> -the sign bit, if the comparison is obviously a sign test. In this case, we only
> -need all but the bottom two bits from %A, and if we gave that mask to SDB it
> -would delete the or instruction for us.
> -
> -//===---------------------------------------------------------------------===//
> -
>   functionattrs doesn't know much about memcpy/memset.  This function should be
>   marked readnone rather than readonly, since it only twiddles local memory, but
>   functionattrs doesn't handle memset/memcpy/memmove aggressively:
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?rev=123203&r1=123202&r2=123203&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Mon Jan 10 18:36:45 2011
> @@ -1693,6 +1693,45 @@
>     return ExtractValueInst::Create(Call, 1, "uadd.overflow");
>   }
>
> +// DemandedBitsLHSMask - When performing a comparison against a constant,
> +// it is possible that not all the bits in the LHS are demanded.  This helper
> +// method computes the mask that IS demanded.

Is the capital IS for emphasis? It looks like a typo.

> +static APInt DemandedBitsLHSMask(ICmpInst&I,
> +                                 unsigned BitWidth, bool isSignCheck) {
> +  if (isSignCheck)
> +    return APInt::getSignBit(BitWidth);
> +
> +  ConstantInt *CI = dyn_cast<ConstantInt>(I.getOperand(1));
> +  if (!CI) return APInt::getAllOnesValue(BitWidth);
> +
> +  APInt RHS = CI->getValue();
> +  APInt Mask(BitWidth, 0);
> +
> +  switch (I.getPredicate()) {
> +  // For a UGT comparison, we don't care about any bits that
> +  // correspond to the trailing ones of the comparand.  The value of these
> +  // bits doesn't impact the outcome of the comparison, because any value
> +  // greater than the RHS must differ in a bit higher than these due to carry.
> +  case ICmpInst::ICMP_UGT: {
> +    unsigned trailingOnes = RHS.countTrailingOnes();
> +    APInt lowBitsSet = APInt::getLowBitsSet(BitWidth, trailingOnes);
> +    return ~lowBitsSet;
> +  }
> +
> +  // Similarly, for a ULT comparison, we don't care about the trailing zeros.
> +  // Any value less than the RHS must differ in a higher bit because of carries.
> +  case ICmpInst::ICMP_ULT: {
> +    unsigned trailingZeros = RHS.countTrailingZeros();
> +    APInt lowBitsSet = APInt::getLowBitsSet(BitWidth, trailingZeros);
> +    return ~lowBitsSet;
> +  }
> +
> +  default:
> +    return APInt::getAllOnesValue(BitWidth);
> +  }
> +
> +  return Mask;
> +}
>
>   Instruction *InstCombiner::visitICmpInst(ICmpInst&I) {
>     bool Changed = false;
> @@ -1830,8 +1869,7 @@
>       APInt Op1KnownZero(BitWidth, 0), Op1KnownOne(BitWidth, 0);
>
>       if (SimplifyDemandedBits(I.getOperandUse(0),
> -                             isSignBit ? APInt::getSignBit(BitWidth)
> -                                       : APInt::getAllOnesValue(BitWidth),
> +                             DemandedBitsLHSMask(I, BitWidth, isSignBit),

I think you can generalize DemandedBitsLHSMask to handle SGT/SLT well 
enough that isSignBit can be removed here.

Nick

>                                Op0KnownZero, Op0KnownOne, 0))
>         return&I;
>       if (SimplifyDemandedBits(I.getOperandUse(1),
>
> Modified: llvm/trunk/test/Transforms/InstCombine/icmp.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/icmp.ll?rev=123203&r1=123202&r2=123203&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/icmp.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/icmp.ll Mon Jan 10 18:36:45 2011
> @@ -192,3 +192,20 @@
>   ; CHECK-NEXT: %cmp = icmp eq i32 %x, 3
>   }
>
> +define i1 @test21(i8 %x, i8 %y) {
> +; CHECK: @test21
> +; CHECK-NOT: or i8
> +; CHECK: icmp ugt
> +  %A = or i8 %x, 1
> +  %B = icmp ugt i8 %A, 3
> +  ret i1 %B
> +}
> +
> +define i1 @test22(i8 %x, i8 %y) {
> +; CHECK: @test22
> +; CHECK-NOT: or i8
> +; CHECK: icmp ult
> +  %A = or i8 %x, 1
> +  %B = icmp ult i8 %A, 4
> +  ret i1 %B
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>