[PATCH] fix incorrect codegen from instcombine (PR17827)

[Kay Tiong Khoo]

Thanks, Evan. Committed as r196129.

After doing more testing with Z3 (see comments in the bug report), I made
the patch a bit more conservative for now. Will try to loosen the
constraints a bit after running more testing.

Bill, please consider this patch for inclusion with 3.4 if it's not too
late - it fixes a case where wrong codegen is produced.


On Sun, Dec 1, 2013 at 11:26 AM, Evan Cheng <evan.cheng at apple.com> wrote:

> 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/20131202/a55abdb5/attachment.html>