<div dir="ltr">Thanks for pointing that out, Hal. Removed links in r197713. Previously referenced Z3 programs pasted into bug report.<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Dec 19, 2013 at 11:24 AM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">----- Original Message -----<br>
> From: "Kay Tiong Khoo" <<a href="mailto:kkhoo@perfwizard.com">kkhoo@perfwizard.com</a>><br>
> To: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> Sent: Thursday, December 19, 2013 12:07:17 PM<br>
> Subject: [llvm] r197705 - Improved fix for PR17827 (instcombine of shift/and/compare).<br>
><br>
> Author: kkhoo<br>
> Date: Thu Dec 19 12:07:17 2013<br>
> New Revision: 197705<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=197705&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=197705&view=rev</a><br>
> Log:<br>
> Improved fix for PR17827 (instcombine of shift/and/compare).<br>
><br>
> This change fixes the case of arithmetic shift right - do not attempt<br>
> to fold that case.<br>
> This change also relaxes the conditions when attempting to fold the<br>
> logical shift right and shift left cases.<br>
><br>
> No additional IR-level test cases included at this time. See<br>
> <a href="http://llvm.org/bugs/show_bug.cgi?id=17827" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=17827</a> for proofs that these are<br>
> correct transformations.<br>
><br>
> Modified:<br>
> llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
><br>
> Modified:<br>
> llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?rev=197705&r1=197704&r2=197705&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?rev=197705&r1=197704&r2=197705&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
> (original)<br>
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Thu<br>
> Dec 19 12:07:17 2013<br>
> @@ -1195,33 +1195,43 @@ Instruction *InstCombiner::visitICmpInst<br>
> ConstantInt *ShAmt;<br>
> ShAmt = Shift ? dyn_cast<ConstantInt>(Shift->getOperand(1)) :<br>
> 0;<br>
><br>
> - // We can fold this as long as we can't shift unknown bits<br>
> - // into the mask. This can happen with signed shift<br>
> - // rights, as they sign-extend. With logical shifts,<br>
> - // we must still make sure the comparison is not signed<br>
> - // because we are effectively changing the<br>
> - // position of the sign bit (PR17827).<br>
> - // TODO: We can relax these constraints a bit more.<br>
> + // This seemingly simple opportunity to fold away a shift<br>
> turns out to<br>
> + // be rather complicated. See PR17827<br>
> + // ( <a href="http://llvm.org/bugs/show_bug.cgi?id=17827" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=17827</a> ) for details.<br>
> if (ShAmt) {<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>
> - Type *ShiftType = Shift->getType();<br>
> - Type *AndType = AndCst->getType();<br>
> -<br>
> - unsigned ShiftBitWidth =<br>
> ShiftType->getPrimitiveSizeInBits();<br>
> - unsigned AndBitWidth = AndType->getPrimitiveSizeInBits();<br>
> -<br>
> - int ShAmtVal = ShiftBitWidth -<br>
> ShAmt->getLimitedValue(ShiftBitWidth);<br>
> -<br>
> - if ((APInt::getHighBitsSet(AndBitWidth, AndBitWidth -<br>
> ShAmtVal) &<br>
> - AndCst->getValue()) == 0)<br>
> + // There may be some constraints that make this possible,<br>
> + // but nothing simple has been discovered yet.<br>
> + CanFold = false;<br>
> + } else if (ShiftOpcode == Instruction::Shl) {<br>
> + // For a left shift, we can fold if the comparison is not<br>
> signed.<br>
> + // We can also fold a signed comparison if the mask value<br>
> and<br>
> + // comparison value are not negative. These constraints<br>
> may not be<br>
> + // obvious, but we can prove that they are correct using<br>
> an SMT<br>
> + // solver such as "Z3" :<br>
> + // <a href="http://rise4fun.com/Z3/DyMp" target="_blank">http://rise4fun.com/Z3/DyMp</a><br>
<br>
</div></div>Kay,<br>
<br>
While I'm a big fan of SMT solvers, I don't think putting these kinds in the source code is a good idea.<br>
<br>
1. While Z3 is a great SMT solver, it is specifically made available for personal and non-commercial use only (see <a href="http://rise4fun.com/termsofuse" target="_blank">http://rise4fun.com/termsofuse</a>), and we have a lot of contributors and users for whom that is an issue.<br>
<br>
2. Who knows what the lifetime of those 'permalinks' will actually be. We should record the relevant details in some place that we control.<br>
<br>
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.<br>
<br>
Thanks again,<br>
Hal<br>
<div class="HOEnZb"><div class="h5"><br>
> + if (!ICI.isSigned() || (!AndCst->isNegative() &&<br>
> !RHS->isNegative()))<br>
> + CanFold = true;<br>
> + } else if (ShiftOpcode == Instruction::LShr) {<br>
> + // For a logical right shift, we can fold if the<br>
> comparison is not<br>
> + // signed. We can also fold a signed comparison if the<br>
> shifted mask<br>
> + // value and the shifted comparison value are not<br>
> negative.<br>
> + // These constraints may not be obvious, but we can prove<br>
> that they<br>
> + // are correct using an SMT solver such as "Z3" :<br>
> + // <a href="http://rise4fun.com/Z3/Tslfh" target="_blank">http://rise4fun.com/Z3/Tslfh</a><br>
> + if (!ICI.isSigned())<br>
> CanFold = true;<br>
> - } else if (ShiftOpcode == Instruction::Shl ||<br>
> - ShiftOpcode == Instruction::LShr) {<br>
> - CanFold = !ICI.isSigned();<br>
> + else {<br>
> + ConstantInt *ShiftedAndCst =<br>
> + cast<ConstantInt>(ConstantExpr::getShl(AndCst,<br>
> ShAmt));<br>
> + ConstantInt *ShiftedRHSCst =<br>
> + cast<ConstantInt>(ConstantExpr::getShl(RHS, ShAmt));<br>
> +<br>
> + if (!ShiftedAndCst->isNegative() &&<br>
> !ShiftedRHSCst->isNegative())<br>
> + CanFold = true;<br>
> + }<br>
> }<br>
><br>
> if (CanFold) {<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">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>
><br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div>