[PATCH] fix incorrect codegen from instcombine (PR17827)

[Kay Tiong Khoo]

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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131125/bea06600/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr17827_r2.patch
Type: application/octet-stream
Size: 5332 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131125/bea06600/attachment.obj>