[llvm-commits] [llvm] r89639 - in /llvm/trunk: lib/Transforms/Scalar/InstructionCombining.cpp test/Transforms/InstCombine/compare-signs.ll

[Chris Lattner]

On Nov 22, 2009, at 7:17 PM, Nick Lewycky wrote:

> Author: nicholas
> Date: Sun Nov 22 21:17:33 2009
> New Revision: 89639
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=89639&view=rev
> Log:
> Reapply r88830 with a bugfix: this transform only applies to icmp eq/ne. This
> fixes part of PR5438.

Thanks for working on this, some thoughts:

> 
> +  // icmp ne A, B is equal to xor A, B when A and B only really have one bit.
> +  // It is also profitable to transform icmp eq into not(xor(A, B)) because that
> +  // may lead to additional simplifications.
> +  if (ICI->isEquality() && CI.getType() == ICI->getOperand(0)->getType()) {
> +    if (const IntegerType *ITy = dyn_cast<IntegerType>(CI.getType())) {
> +      uint32_t BitWidth = ITy->getBitWidth();
> +      if (BitWidth > 1) {

If this is a zext from bool, the result type can't be i1.  The source and dest of a zext can't be the same type, so this 'if' seems unneeded.

> +        Value *LHS = ICI->getOperand(0);
> +        Value *RHS = ICI->getOperand(1);
> +
> +        APInt KnownZeroLHS(BitWidth, 0), KnownOneLHS(BitWidth, 0);
> +        APInt KnownZeroRHS(BitWidth, 0), KnownOneRHS(BitWidth, 0);
> +        APInt TypeMask(APInt::getHighBitsSet(BitWidth, BitWidth-1));

Why isn't TypeMask looking at all the bits?

> +        ComputeMaskedBits(LHS, TypeMask, KnownZeroLHS, KnownOneLHS);
> +        ComputeMaskedBits(RHS, TypeMask, KnownZeroRHS, KnownOneRHS);
> +
> +        if (KnownZeroLHS.countLeadingOnes() == BitWidth-1 &&
> +            KnownZeroRHS.countLeadingOnes() == BitWidth-1) {

This check seems over-constrained.  This xforms should apply when it is one of *any* bit set.  For example, this:

entry:
        %0 = and i32 %a, 8            ; <i32> [#uses=1]
        %1 = and i32 %b, 8            ; <i32> [#uses=1]
        %2 = icmp eq i32 %0, %1         ; <i1> [#uses=1]
        %3 = zext i1 %2 to i32          ; <i32> [#uses=1]
        ret i32 %3
}

should also be transformed.  If you intend to keep it written this way, it would be more natural to use MaskedValueIsZero instead of ComputeMaskedBits, however, generalizing this should be easy, so please do :)

-Chris