[llvm] r197705 - Improved fix for PR17827 (instcombine of shift/and/compare).

[Hal Finkel]

----- Original Message -----
> From: "Kay Tiong Khoo" <kkhoo at perfwizard.com>
> To: llvm-commits at cs.uiuc.edu
> Sent: Thursday, December 19, 2013 12:07:17 PM
> Subject: [llvm] r197705 - Improved fix for PR17827 (instcombine of	shift/and/compare).
> 
> Author: kkhoo
> Date: Thu Dec 19 12:07:17 2013
> New Revision: 197705
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=197705&view=rev
> Log:
> Improved fix for PR17827 (instcombine of shift/and/compare).
> 
> This change fixes the case of arithmetic shift right - do not attempt
> to fold that case.
> This change also relaxes the conditions when attempting to fold the
> logical shift right and shift left cases.
> 
> No additional IR-level test cases included at this time. See
> http://llvm.org/bugs/show_bug.cgi?id=17827 for proofs that these are
> correct transformations.
> 
> Modified:
>     llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
> 
> Modified:
> llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?rev=197705&r1=197704&r2=197705&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
> (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Thu
> Dec 19 12:07:17 2013
> @@ -1195,33 +1195,43 @@ Instruction *InstCombiner::visitICmpInst
>        ConstantInt *ShAmt;
>        ShAmt = Shift ? dyn_cast<ConstantInt>(Shift->getOperand(1)) :
>        0;
>  
> -      // We can fold this as long as we can't shift unknown bits
> -      // into the mask. This can happen with signed shift
> -      // rights, as they sign-extend. With logical shifts,
> -      // we must still make sure the comparison is not signed
> -      // because we are effectively changing the
> -      // position of the sign bit (PR17827).
> -      // TODO: We can relax these constraints a bit more.
> +      // This seemingly simple opportunity to fold away a shift
> turns out to
> +      // be rather complicated. See PR17827
> +      // ( http://llvm.org/bugs/show_bug.cgi?id=17827 ) for details.
>        if (ShAmt) {
>          bool CanFold = false;
>          unsigned ShiftOpcode = Shift->getOpcode();
>          if (ShiftOpcode == Instruction::AShr) {
> -          // To test for the bad case of the signed shr, see if any
> -          // of the bits shifted in could be tested after the mask.
> -          Type *ShiftType = Shift->getType();
> -          Type *AndType = AndCst->getType();
> -
> -          unsigned ShiftBitWidth =
> ShiftType->getPrimitiveSizeInBits();
> -          unsigned AndBitWidth = AndType->getPrimitiveSizeInBits();
> -
> -          int ShAmtVal = ShiftBitWidth -
> ShAmt->getLimitedValue(ShiftBitWidth);
> -
> -          if ((APInt::getHighBitsSet(AndBitWidth, AndBitWidth -
> ShAmtVal) &
> -               AndCst->getValue()) == 0)
> +          // There may be some constraints that make this possible,
> +          // but nothing simple has been discovered yet.
> +          CanFold = false;
> +        } else if (ShiftOpcode == Instruction::Shl) {
> +          // For a left shift, we can fold if the comparison is not
> signed.
> +          // We can also fold a signed comparison if the mask value
> and
> +          // comparison value are not negative. These constraints
> may not be
> +          // obvious, but we can prove that they are correct using
> an SMT
> +          // solver such as "Z3" :
> +          // http://rise4fun.com/Z3/DyMp

Kay,

While I'm a big fan of SMT solvers, I don't think putting these kinds in the source code is a good idea.

 1. While Z3 is a great SMT solver, it is specifically made available for personal and non-commercial use only (see http://rise4fun.com/termsofuse), and we have a lot of contributors and users for whom that is an issue.

 2. Who knows what the lifetime of those 'permalinks' will actually be. We should record the relevant details in some place that we control.

Please record whatever input or output is relevant in the bug report, and remove the references to Z3 and the links from the source code.

Thanks again,
Hal

> +          if (!ICI.isSigned() || (!AndCst->isNegative() &&
> !RHS->isNegative()))
> +            CanFold = true;
> +        } else if (ShiftOpcode == Instruction::LShr) {
> +          // For a logical right shift, we can fold if the
> comparison is not
> +          // signed. We can also fold a signed comparison if the
> shifted mask
> +          // value and the shifted comparison value are not
> negative.
> +          // These constraints may not be obvious, but we can prove
> that they
> +          // are correct using an SMT solver such as "Z3" :
> +          // http://rise4fun.com/Z3/Tslfh
> +          if (!ICI.isSigned())
>              CanFold = true;
> -        } else if (ShiftOpcode == Instruction::Shl ||
> -                   ShiftOpcode == Instruction::LShr) {
> -          CanFold = !ICI.isSigned();
> +          else {
> +            ConstantInt *ShiftedAndCst =
> +              cast<ConstantInt>(ConstantExpr::getShl(AndCst,
> ShAmt));
> +            ConstantInt *ShiftedRHSCst =
> +              cast<ConstantInt>(ConstantExpr::getShl(RHS, ShAmt));
> +
> +            if (!ShiftedAndCst->isNegative() &&
> !ShiftedRHSCst->isNegative())
> +              CanFold = true;
> +          }
>          }
>  
>          if (CanFold) {
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory