[PATCH] fix incorrect codegen from instcombine (PR17827)

[Evan Cheng]

Sorry about the delay. LGTM.

Evan
On Nov 25, 2013, at 10:55 AM, Kay Tiong Khoo <kkhoo at perfwizard.com> wrote:

> Thanks, Evan. Please see the revised patch here. I cached the opcode value in a local variable since we're using it all over the place. I also adjusted the if-check as you suggested and check each of the individual opcode cases rather than mixing 'isArithmeticShift' in there and assuming LShr on the last case.
> 
> I'd certainly like to try some stronger verification for this and other bugs. I've cc'd you and Nuno on bug 17827 and hope to continue the discussion there.
> 
> I don't think we should hold up this patch waiting for that though. I'd think this patch is also a 3.4 branch candidate since we're generating wrong code. Please let me know if you agree and if it's ok to commit.
> 
> Thanks!
> 
> 
> Index: lib/Transforms/InstCombine/InstCombineCompares.cpp
> ===================================================================
> --- lib/Transforms/InstCombine/InstCombineCompares.cpp  (revision 195371)
> +++ lib/Transforms/InstCombine/InstCombineCompares.cpp  (working copy)
> @@ -1198,11 +1198,15 @@
>        Type *AndTy = AndCST->getType();          // Type of the and.
> 
>        // We can fold this as long as we can't shift unknown bits
> -      // into the mask.  This can only happen with signed shift
> -      // rights, as they sign-extend.
> +      // into the mask. This can happen with signed shift
> +      // rights, as they sign-extend. With a left shift and a
> +      // signed comparison, we must also make sure that 'nsw' is
> +      // set on the shift because we are effectively changing the
> +      // position of the sign bit (PR17827).
>        if (ShAmt) {
> -        bool CanFold = Shift->isLogicalShift();
> -        if (!CanFold) {
> +        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.
>            uint32_t TyBits = Ty->getPrimitiveSizeInBits();
> @@ -1212,19 +1216,22 @@
>            if ((APInt::getHighBitsSet(BitWidth, BitWidth-ShAmtVal) &
>                 AndCST->getValue()) == 0)
>              CanFold = true;
> +        } else if (ShiftOpcode == Instruction::Shl) {
> +          CanFold = Shift->hasNoSignedWrap() || !ICI.isSigned();
> +        } else if (ShiftOpcode == Instruction::LShr){
> +          CanFold = true;
>          }
> 
>          if (CanFold) {
>            Constant *NewCst;
> -          if (Shift->getOpcode() == Instruction::Shl)
> +          if (ShiftOpcode == Instruction::Shl)
>              NewCst = ConstantExpr::getLShr(RHS, ShAmt);
>            else
>              NewCst = ConstantExpr::getShl(RHS, ShAmt);
> 
>            // Check to see if we are shifting out any of the bits being
>            // compared.
> -          if (ConstantExpr::get(Shift->getOpcode(),
> -                                       NewCst, ShAmt) != RHS) {
> +          if (ConstantExpr::get(ShiftOpcode, NewCst, ShAmt) != RHS) {
>              // If we shifted bits out, the fold is not going to work out.
>              // As a special case, check to see if this means that the
>              // result is always true or false now.
> @@ -1235,7 +1242,7 @@
>            } else {
>              ICI.setOperand(1, NewCst);
>              Constant *NewAndCST;
> -            if (Shift->getOpcode() == Instruction::Shl)
> +            if (ShiftOpcode == Instruction::Shl)
>                NewAndCST = ConstantExpr::getLShr(AndCST, ShAmt);
>              else
>                NewAndCST = ConstantExpr::getShl(AndCST, ShAmt);
> 
> 
> 
> 
> On Thu, Nov 21, 2013 at 6:34 PM, Evan Cheng <evan.cheng at apple.com> wrote:
> I looked at the analysis in the bug report and I think I agree with your assessment. Perhaps you can use Nuno's technique to verify the optimization with a SMT solver?
> 
> As for the patch itself, (assuming the analysis is correct), the patch is trivially correct. The only adjustment I would make is mechanical:
> 
> +        } else { // logical shift left or right
> +          if (Shift->getOpcode() == Instruction::Shl) {
> +            CanFold = Shift->hasNoSignedWrap() || !ICI.isSigned();
> +          } else { // logical shift right
> +            CanFold = true;
> +          }
>          }
> 
> I would have combined the else and the first if to
> } else if (Shift->getOpcode() == Instruction::Shl) {
> 
> Evan
> 
> On Nov 21, 2013, at 11:46 AM, Kay Tiong Khoo <kkhoo at perfwizard.com> wrote:
> 
>> Detailed description of problem in instcombine's compare optimizer:
>> http://llvm.org/bugs/show_bug.cgi?id=17827
>> 
>> Fix suggested by Henrique Santos via dev mailing list:
>> http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-November/067822.html
>> <pr17827.patch>_______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> <pr17827_r2.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131201/53028a50/attachment.html>