<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">+ Correct commits list.<br class=""><div><blockquote type="cite" class=""><div class="">On Sep 30, 2015, at 6:15 PM, Ad</div></blockquote>The bug is also present in compiler-rt <a href="http://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/builtins/udivsi3.c" class="">http://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/builtins/udivsi3.c</a><br class=""><blockquote type="cite" class=""><div class="">itya Nandakumar <<a href="mailto:aditya_nandakumar@apple.com" class="">aditya_nandakumar@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Sorry - my client completely lost this thread and I didn’t get notified.<div class=""><br class=""></div><div class=""><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class="">> I must not understand correctly. It sounds like you are positing the following:
> 1) There is a logic flaw that this corrects.</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class="">Yes. This is what the patch fixes.</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class="">The expansion basically does this</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class="">divide(num, denom)</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class="">{</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class=""><span class="Apple-tab-span" style="white-space:pre"> </span>if (num == 0) return 0</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class=""><span class="Apple-tab-span" style="white-space:pre"> </span>if (denim == 0) undefined.</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class=""><span class="Apple-tab-span" style="white-space:pre"> </span>if (denom == 1) return num // This step is checked incorrectly.</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class=""><span class="Apple-tab-span" style="white-space:pre"> </span>{</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class=""><span class="Apple-tab-span" style="white-space:pre"> </span>// Loop expansion for the division</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class=""><span class="Apple-tab-span" style="white-space:pre"> </span>}</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class="">To test if the denominator == 1, we should be doing if ctlz(denom) == (num_bits - 1)</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class="">What it actually tests is if ctlz(denom) - ctlz(num) == (num_bits - 1).</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class="">So wrt correctness, when the divisor is 1, we should just be exiting the expansion early with the numerator, but instead we go into the loop and get the numerator.</pre><pre style="white-space: pre-wrap; background-color: rgb(255, 255, 255);" class="">The end result is still correct but we skipped the short cutting logic and wasted time in the loop.
> 2) There is no bit pattern for the inputs/output that demonstrates this flaw.</pre><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class="">On Jul 6, 2015, at 4:43 PM, Aditya Nandakumar <<a href="mailto:aditya_nandakumar@apple.com" class="">aditya_nandakumar@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">The problem is the check is a special case for handling X/1 - even if the special case is not being used, the expansion into the loop should still produce a valid quotient. It would be hard to test before and after for correctness.<br class="">The problem with the out of tree target is likely in the codegen of the expansion into the loop - and changing the special case of divide by 1 only fixes that case.<br class=""><blockquote type="cite" class="">On Jul 6, 2015, at 4:35 PM, Michael Ilseman <<a href="mailto:milseman@apple.com" class="">milseman@apple.com</a>> wrote:<br class=""><br class="">Then can you sanitize and commit such a test case?<br class=""><br class=""><blockquote type="cite" class="">On Jul 6, 2015, at 4:26 PM, Aditya Nandakumar <<a href="mailto:aditya_nandakumar@apple.com" class="">aditya_nandakumar@apple.com</a>> wrote:<br class=""><br class="">I don’t have a test case before and after for in-tree target but it fixed test for out of tree target.<br class="">The logic is what I am pointing out is wrong - we need to check if CTLZ(divisor) == Num_bits - 1 rather than CTLZ(divisor) - CTLZ(dividend).<br class=""><blockquote type="cite" class="">On Jul 6, 2015, at 2:29 PM, Michael Ilseman <<a href="mailto:milseman@apple.com" class="">milseman@apple.com</a>> wrote:<br class=""><br class="">Do you have a test case that demonstrates the failure before-hand, and the fix after? <br class=""><br class=""><blockquote type="cite" class="">On Jul 2, 2015, at 12:26 PM, Aditya Nandakumar <<a href="mailto:aditya_nandakumar@apple.com" class="">aditya_nandakumar@apple.com</a>> wrote:<br class=""><br class=""><DiffIntUnsignedDivision.patch><br class=""></blockquote><br class=""></blockquote><br class=""></blockquote><br class=""></blockquote><br class=""></div></div></blockquote></div><br class=""></div></div></div></blockquote></div><br class=""></body></html>