<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Sorry about the delay. LGTM.<div><br></div><div>Evan<br><div><div>On Nov 25, 2013, at 10:55 AM, Kay Tiong Khoo <<a href="mailto:kkhoo@perfwizard.com">kkhoo@perfwizard.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr"><div><div><div>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.<br>
<br></div>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.<br><br></div>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.<br>
<br></div>Thanks!<br><div><div><br><br>Index: lib/Transforms/InstCombine/InstCombineCompares.cpp<br>===================================================================<br>--- lib/Transforms/InstCombine/InstCombineCompares.cpp (revision 195371)<br>
+++ lib/Transforms/InstCombine/InstCombineCompares.cpp (working copy)<br>@@ -1198,11 +1198,15 @@<br> Type *AndTy = AndCST->getType(); // Type of the and.<br><br> // We can fold this as long as we can't shift unknown bits<br>
- // into the mask. This can only happen with signed shift<br>- // rights, as they sign-extend.<br>+ // into the mask. This can happen with signed shift<br>+ // rights, as they sign-extend. With a left shift and a<br>
+ // signed comparison, we must also make sure that 'nsw' is<br>+ // set on the shift because we are effectively changing the<br>+ // position of the sign bit (PR17827).<br> if (ShAmt) {<br>- bool CanFold = Shift->isLogicalShift();<br>
- if (!CanFold) {<br>+ bool CanFold = false;<br>+ unsigned ShiftOpcode = Shift->getOpcode();<br>+ if (ShiftOpcode == Instruction::AShr) {<br> // To test for the bad case of the signed shr, see if any<br>
// of the bits shifted in could be tested after the mask.<br> uint32_t TyBits = Ty->getPrimitiveSizeInBits();<br>@@ -1212,19 +1216,22 @@<br> if ((APInt::getHighBitsSet(BitWidth, BitWidth-ShAmtVal) &<br>
AndCST->getValue()) == 0)<br> CanFold = true;<br>+ } else if (ShiftOpcode == Instruction::Shl) {<br>+ CanFold = Shift->hasNoSignedWrap() || !ICI.isSigned();<br>+ } else if (ShiftOpcode == Instruction::LShr){<br>
+ CanFold = true;<br> }<br><br> if (CanFold) {<br> Constant *NewCst;<br>- if (Shift->getOpcode() == Instruction::Shl)<br>+ if (ShiftOpcode == Instruction::Shl)<br> NewCst = ConstantExpr::getLShr(RHS, ShAmt);<br>
else<br> NewCst = ConstantExpr::getShl(RHS, ShAmt);<br><br> // Check to see if we are shifting out any of the bits being<br> // compared.<br>- if (ConstantExpr::get(Shift->getOpcode(),<br>
- NewCst, ShAmt) != RHS) {<br>+ if (ConstantExpr::get(ShiftOpcode, NewCst, ShAmt) != RHS) {<br> // If we shifted bits out, the fold is not going to work out.<br> // As a special case, check to see if this means that the<br>
// result is always true or false now.<br>@@ -1235,7 +1242,7 @@<br> } else {<br> ICI.setOperand(1, NewCst);<br> Constant *NewAndCST;<br>- if (Shift->getOpcode() == Instruction::Shl)<br>
+ if (ShiftOpcode == Instruction::Shl)<br> NewAndCST = ConstantExpr::getLShr(AndCST, ShAmt);<br> else<br> NewAndCST = ConstantExpr::getShl(AndCST, ShAmt);<br><br><br></div>
</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Nov 21, 2013 at 6:34 PM, Evan Cheng <span dir="ltr"><<a href="mailto:evan.cheng@apple.com" target="_blank">evan.cheng@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">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?<div>
<br></div><div>As for the patch itself, (assuming the analysis is correct), the patch is trivially correct. The only adjustment I would make is mechanical:</div><div><br></div><div><div>+ } else { // logical shift left or right</div>
<div>+ if (Shift->getOpcode() == Instruction::Shl) {</div><div>+ CanFold = Shift->hasNoSignedWrap() || !ICI.isSigned();</div><div>+ } else { // logical shift right</div><div>+ CanFold = true;</div>
<div>+ }</div><div> }</div><div><br></div><div>I would have combined the else and the first if to</div><div>} else if (Shift->getOpcode() == Instruction::Shl) {</div><div><br></div><div>Evan</div><div>
<br></div><div><div><div class="h5"><div>On Nov 21, 2013, at 11:46 AM, Kay Tiong Khoo <<a href="mailto:kkhoo@perfwizard.com" target="_blank">kkhoo@perfwizard.com</a>> wrote:</div><br></div></div><blockquote type="cite">
<div><div class="h5"><div dir="ltr"><div>Detailed description of problem in instcombine's compare optimizer:<br><a href="http://llvm.org/bugs/show_bug.cgi?id=17827" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=17827</a><br>
<br></div>Fix suggested by Henrique Santos via dev mailing list:<br>
<a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-November/067822.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-November/067822.html</a><br></div>
</div></div><span><pr17827.patch></span>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div></blockquote></div><br></div>
<span><pr17827_r2.patch></span></blockquote></div><br></div></body></html>