<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 20 February 2015 at 16:31, Sanjoy Das <span dir="ltr"><<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> Sanjoy, I’m not entirely sure - it’s a bit late for me to have my effective thinking cap on. At the moment it does seem to me that if we swapped ZExt for Sext in multiply() things might just come out correctly, but I’d need to prove it to myself.<br>
<br>
</span>My suggestion would be (echoing what Hal said) to simply make<br>
ConstantRange::multiply smarter. There is no difference between<br>
signed and unsigned multiplication in 2's complement, so there<br>
shouldn't be a difference in ConstantRange's multiplication either.<br></blockquote><div><br></div><div>Correct. This follows from the fact that the resulting constant range is any of the smallest ranges that cover all values you get multiplying each value in the LHS range with each value in the RHS range. That's a sign-independent definition.</div><div><br></div><div>The one case where you may get signed vs. unsigned differences is when you have the choice between multiple different constant ranges that cover all values and you can choose where the gap in the constant range is. Suppose I asked for multiply(i8 [0, 128), i8[2, 3)) (aka. all even numbers in i8), you could choose between i8 [0, 255) and [-128, 127). Of course, other representations like [2, 1) are equally valid (there's 128 of them) but I don't think anyone actually needs this.</div><div><br></div><div>James, it's hard to tell what you need when the patch doesn't include an update to the unit test?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
For instance, [-1, 2) * [-1, 2) should be [-1, 2) in both signed and<br>
unsigned interpretations (I don't know what ConstantRange::multiply<br>
does today, it may be possible that it already handles this).<br></blockquote><div><br></div><div>It's hard to do multiplication of ConstantRanges precisely, and multiply does have a TODO requesting improvement. The problems start once you have multiplies that can wrap. What you need to solve for is the max and min of each of the multiplies (cartesian product) in modulo 2^n space. Because we need to push the resulting set into a constant range, what it looks like is "how far > 0 is the lowest value" and "how far < UINT_MAX is the highest value", with no interesting information about the values in the middle.</div><div><br></div><div>Nick</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
> On 20 Feb 2015, at 23:48, Sanjoy Das <<a href="mailto:sanjoy@playingwithpointers.com">sanjoy@playingwithpointers.com</a>> wrote:<br>
><br>
>> Actually, I don't think there is any need for a separate smultiply<br>
>> when we're working on two's complement. IMHO, the right solution is<br>
>> to just make ConstantRange::multiply smarter about handing cases like<br>
>> [-1,2) * [-1,2) etc.<br>
>><br>
>> -- Sanjoy<br>
>><br>
>> On Fri, Feb 20, 2015 at 3:40 PM, Sanjoy Das<br>
>> <<a href="mailto:sanjoy@playingwithpointers.com">sanjoy@playingwithpointers.com</a>> wrote:<br>
>>> I don't think so:<br>
>>><br>
>>> [-3,0) * [-3, 0) should be [1, 10) but using your formula I get [3,<br>
>>> <something>).<br>
>>><br>
>>> On Fri, Feb 20, 2015 at 3:31 PM, Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>> wrote:<br>
>>>> ----- Original Message -----<br>
>>>>> From: "Sanjoy Das" <<a href="mailto:sanjoy@playingwithpointers.com">sanjoy@playingwithpointers.com</a>><br>
>>>>> To: <a href="mailto:reviews%2BD7789%2Bpublic%2Bffa7549bb8a7b441@reviews.llvm.org">reviews+D7789+public+ffa7549bb8a7b441@reviews.llvm.org</a><br>
>>>>> Cc: "james molloy" <<a href="mailto:james.molloy@arm.com">james.molloy@arm.com</a>>, "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>>, "Commit Messages and Patches for LLVM"<br>
>>>>> <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>><br>
>>>>> Sent: Friday, February 20, 2015 5:11:46 PM<br>
>>>>> Subject: Re: [PATCH] [ConstantRange] Add a smultiply method for signed multiply<br>
>>>>><br>
>>>>>> ConstantRange<br>
>>>>>> +ConstantRange::smultiply(const ConstantRange &Other) const {<br>
>>>>>> + // TODO: If either operand is a single element and the multiply<br>
>>>>>> is known to<br>
>>>>>> + // be non-wrapping, round the result min and max value to the<br>
>>>>>> appropriate<br>
>>>>>> + // multiple of that element. If wrapping is possible, at least<br>
>>>>>> adjust the<br>
>>>>>> + // range according to the greatest power-of-two factor of the<br>
>>>>>> single element.<br>
>>>>>> +<br>
>>>>>> + if (isEmptySet() || Other.isEmptySet())<br>
>>>>>> + return ConstantRange(getBitWidth(), /*isFullSet=*/false);<br>
>>>>>> +<br>
>>>>>> + APInt this_min = getSignedMin().sext(getBitWidth() * 2);<br>
>>>>>> + APInt this_max = getSignedMax().sext(getBitWidth() * 2);<br>
>>>>>> + APInt Other_min = Other.getSignedMin().sext(getBitWidth() * 2);<br>
>>>>>> + APInt Other_max = Other.getSignedMax().sext(getBitWidth() * 2);<br>
>>>>>> +<br>
>>>>>> + ConstantRange Result_zext = ConstantRange(this_min * Other_min,<br>
>>>>>> + this_max * Other_max +<br>
>>>>>> 1);<br>
>>>>>> + return Result_zext.truncate(getBitWidth());<br>
>>>>><br>
>>>>> I'm not sure if this is correct: if we multiply [-1, 2) with itself,<br>
>>>>> won't this return [1, 2)? Shouldn't [-1,2) * [-1,2) be [-1,2)?<br>
>>>><br>
>>>> Indeed. The minimum should not be this_min * Other_min, but rather min(this_min * Other_max, this_max * Other_min), right?<br>
>>>><br>
>>>> -Hal<br>
>>>><br>
>>>>><br>
>>>>> -- Sanjoy<br>
>>>>><br>
>>>><br>
>>>> --<br>
>>>> Hal Finkel<br>
>>>> Assistant Computational Scientist<br>
>>>> Leadership Computing Facility<br>
>>>> Argonne National Laboratory<br>
>><br>
><br>
><br>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.<br>
><br>
> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590<br>
> ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782<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>
</div></div></blockquote></div></div></div>