[Review]: Fix Integer Division Expansion special case of divide by 1

[Aditya Nandakumar via llvm-commits]

+ Correct commits list.
> On Sep 30, 2015, at 6:15 PM, Ad
The bug is also present in compiler-rt http://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/builtins/udivsi3.c
> itya Nandakumar <aditya_nandakumar at apple.com> wrote:
> 
> Sorry - my client completely lost this thread and I didn’t get notified.
> 
> > I must not understand correctly. It sounds like you are positing the following:
> > 1) There is a logic flaw that this corrects.
> Yes. This is what the patch fixes.
> The expansion basically does this
> divide(num, denom)
> {
> 	if (num == 0) return 0
> 	if (denim == 0) undefined.
> 	if (denom == 1) return num // This step is checked incorrectly.
> 	{
> 		// Loop expansion for the division
> 	}
> To test if the denominator == 1, we should be doing if ctlz(denom) == (num_bits - 1)
> What it actually tests is if ctlz(denom) - ctlz(num) == (num_bits - 1).
> 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.
> 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.
> 
>> On Jul 6, 2015, at 4:43 PM, Aditya Nandakumar <aditya_nandakumar at apple.com <mailto:aditya_nandakumar at apple.com>> wrote:
>> 
>> 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.
>> 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.
>>> On Jul 6, 2015, at 4:35 PM, Michael Ilseman <milseman at apple.com <mailto:milseman at apple.com>> wrote:
>>> 
>>> Then can you sanitize and commit such a test case?
>>> 
>>>> On Jul 6, 2015, at 4:26 PM, Aditya Nandakumar <aditya_nandakumar at apple.com <mailto:aditya_nandakumar at apple.com>> wrote:
>>>> 
>>>> I don’t have a test case before and after for in-tree target but it fixed test for out of tree target.
>>>> 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).
>>>>> On Jul 6, 2015, at 2:29 PM, Michael Ilseman <milseman at apple.com <mailto:milseman at apple.com>> wrote:
>>>>> 
>>>>> Do you have a test case that demonstrates the failure before-hand, and the fix after? 
>>>>> 
>>>>>> On Jul 2, 2015, at 12:26 PM, Aditya Nandakumar <aditya_nandakumar at apple.com <mailto:aditya_nandakumar at apple.com>> wrote:
>>>>>> 
>>>>>> <DiffIntUnsignedDivision.patch>
>>>>> 
>>>> 
>>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151001/0b5dff26/attachment.html>