<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 21 February 2015 at 03:58, James Molloy <span dir="ltr"><<a href="mailto:James.Molloy@arm.com" target="_blank">James.Molloy@arm.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">
Hi Nick, all,
<div><br>
</div>
<div>Firstly apologies for not creating a unit test. I thought this was totally trivial and it turns out it’s completely not.</div>
<div><br>
</div>
<div>When wrapping, it is sufficient for my use case to know that we’ve wrapped. I don’t care about what we represent a wrapped value as (where we pick the gap).</div>
<div>
<div><br>
</div>
<div>Discounting wrapping for the moment, it seems to me that while multiplication is signedness-independent, minimum and maximum are not. </div>
</div>
<div><br>
</div>
<div>[-2,0) * [-4,4)</div>
<div>Consider the trivial case: i8 [254,0) * i8 [252,4). In signed arithmetic,we get:</div>
<div>a[0] * b[0] = 8</div>
<div>a[1] * b[1] = 0</div>
<div>a[0] * b[1] = -8</div>
<div>a[1] * b[0] = 0</div>
<div><br>
</div>
<div>min: -8, max: 8</div>
<div><br>
</div>
<div>In unsigned arithmetic we get:</div>
<div>a[0] * b[0] = 8</div>
<div>a[1] * b[1] = 0</div>
<div>a[0] * b[1] = 248</div>
<div>a[1] * b[0] = 0</div>
<div><br>
</div>
<div>min: 8, max: 248 ( =-8 )</div>
<div><br>
</div>
<div>So depending on the signedness of the minimum/maximum operation, we get different results. Therefore, we do need a signed version of multiply.</div>
<div><br>
</div>
<div>That’s as far as my logic takes me, could someone please point out where I’ve gone wrong?</div></div></blockquote><div><br></div><div>Fantastic example!</div><div><br></div><div>The total set of values is {0, 1, 2, 3, 4, 6, 8, 250, 252, 254, 255}. The best range to cover that is [250, 9), but we calculate full-set because of the truncation step.</div><div><br></div><div>The multiply function shouldn't have signed or unsigned variations in the API because there is only one best resulting range either way, but you've clearly identified a deficiency. ConstantRange::multiply could compute it twice once with zext and unsigned min/max and again with sext and signed min/max, then choose the one with the smaller range. Maybe there's an even better algorithm.</div><div><br></div><div>Nick</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><br>
</div>
<div>Cheers,</div>
<div><br>
</div>
<div>James</div><div><div>
<div><br>
<div>
<div>On 21 Feb 2015, at 02:11, Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>> wrote:</div>
<br>
<blockquote type="cite">
<div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">On 20 February 2015 at 17:49, Nick Lewycky<span> </span><span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span><span> </span>wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote"><span>On 20 February 2015 at 17:41, Sanjoy Das<span> </span><span dir="ltr"><<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.com</a>></span><span> </span>wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span>> It's hard to do multiplication of ConstantRanges precisely, and multiply<br>
> does have a TODO requesting improvement. The problems start once you have<br>
> multiplies that can wrap. What you need to solve for is the max and min of<br>
> each of the multiplies (cartesian product) in modulo 2^n space. Because we<br>
> need to push the resulting set into a constant range, what it looks like is<br>
> "how far > 0 is the lowest value" and "how far < UINT_MAX is the highest<br>
> value", with no interesting information about the values in the middle.<br>
<br>
</span>Just to make sure I understand you: this problem isn't really limited<br>
to cases that actually wrap -- i32 [1, 4) * i32 [3, 4) == i32 {3, 6,<br>
9} and this structure is lost when we coerce the result to [3, 10).<br>
</blockquote>
<div><br>
</div>
</span>
<div>I wasn't considering that a problem. ConstantRange::multiply today precisely produces the range [3, 10) in that case. There is no improvement to be made.</div>
<div><br>
</div>
<div>Consider i8 [1, 4) * i8 [9, 10).</div>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>Uhm, I meant to say "i8 [1, 86) * i8 [9, 10)". That's quite a typo.</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>The best possible range is [1, 253), but our implementation will return the full set. That's really hard to get right. We extend the bit range to twice the bit range so that we can do the multiply algebraically, then we call truncate. Truncate correctly
detects that we cover the whole span of integers in the bitwidth and returns the full set.</div>
<span><font color="#888888">
<div><br>
</div>
<div>Nick</div>
</font></span>
<div>
<div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
I wonder how crazy it is to have the notion of a first-class<br>
ConstantRange "expression" (i.e. remembering that a ConstantRange is<br>
i32 [1, 4) * i32 [3, 4) and not eagerly flattening it).<br>
</blockquote>
<div> <br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><font color="#888888">-- Sanjoy<br>
</font></span>
<div><br>
><br>
> Nick<br>
><br>
>><br>
>> > On 20 Feb 2015, at 23:48, Sanjoy Das <<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.com</a>><br>
>> > 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" target="_blank">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" target="_blank">hfinkel@anl.gov</a>> wrote:<br>
>> >>>> ----- Original Message -----<br>
>> >>>>> From: "Sanjoy Das" <<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.com</a>><br>
>> >>>>> To:<span> </span><a href="mailto:reviews%2BD7789%2Bpublic%2Bffa7549bb8a7b441@reviews.llvm.org" target="_blank">reviews+D7789+public+ffa7549bb8a7b441@reviews.llvm.org</a><br>
>> >>>>> Cc: "james molloy" <<a href="mailto:james.molloy@arm.com" target="_blank">james.molloy@arm.com</a>>, "Hal Finkel"<br>
>> >>>>> <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>>, "Commit Messages and Patches for LLVM"<br>
>> >>>>> <<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">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<br>
>> >>>>> 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<br>
>> >>>>> 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<br>
>> >>>> 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<br>
>> > confidential and may also be privileged. If you are not the intended<br>
>> > recipient, please notify the sender immediately and do not disclose the<br>
>> > contents to any other person, use it for any purpose, or store or copy the<br>
>> > information in any medium. Thank you.<br>
>> ><br>
>> > ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ,<br>
>> > Registered in England & Wales, Company No: 2557590<br>
>> > ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1<br>
>> > 9NJ, Registered in England & Wales, Company No: 2548782<br>
>> ><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>><span> </span><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>><span> </span><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div>
</blockquote>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
<br>
<font face="Arial" color="Black">-- 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>
</font>
</div></div></div>
</blockquote></div><br></div></div>