<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Sat, Jun 27, 2015 at 2:49 AM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<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"><br>
> On 2015-Jun-26, at 11:42, Philip Reames <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>> wrote:<br>
><br>
><br>
><br>
> On 06/24/2015 10:04 AM, Duncan P. N. Exon Smith wrote:<br>
>>> On 2015 Jun 23, at 08:14, Paweł Bylica <<a href="mailto:chfast@gmail.com" target="_blank">chfast@gmail.com</a>> wrote:<br>
>>><br>
>>> Hi chandlerc,<br>
>>><br>
>>> This patch changes the way APInt is compared with a value of type uint64_t.<br>
>>> Before the uint64_t value was truncated to the size of APInt before comparison.<br>
>>> Now the comparison takes into account full 64-bit precision.<br>
>>><br>
>>> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10655&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=yL_TK7ks5jZJUUa2nH5nAW2wn4VBN8Tt9ZalXS-zlPg&s=bKZgEKrmdS-K5SRDiyLAGnoEhQnEyJPd3LTzYpmP540&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10655</a><br>
>>><br>
>>> Files:<br>
>>> include/llvm/ADT/APInt.h<br>
>>> unittests/ADT/APIntTest.cpp<br>
>>><br>
>> You never got a response from your llvmdev post. There are two ways to<br>
>> go here:<br>
>><br>
>> 1. Assert that the value is in the range of BitWidth.<br>
>> 2. Extend this to 64-bits and compare.<br>
>><br>
>> I'm inclined to agree that (2) is more useful -- developers can opt-in<br>
>> to the old behaviour by hand-constructing an `APInt()` -- but I'd like<br>
>> to hear from someone else before this is committed.<br>
> I think either semantic is reasonable. I'd have a personal preference for (1), but will defer to interested parties to make the actual decision. Just make sure you *clearly* document the result. In particular, I don't see header comments being updated in the patch below.<br>
<br>
Anyone else? Pawel, any particular reason you didn't go for (1)?<br></blockquote><div><br></div><div>I believe (2) is much more useful because of the following pattern can be found all over the code:</div><div><br></div><div><div> uint64_t BitWidth = getTypeSizeInBits(U->getType());</div><div> if (CI->getValue().uge(BitWidth))<br></div><div> break;</div></div><div><br></div><div>Sometimes also incorrectly expressed as CI->getZExtValue() >= BitWidth.</div><div><br></div><div>So using pure .uge without semantic change will not be correct either. The shortest correct expression with current semantics is CI->getValue().zextOfSelf(64).uge(BitWitdh). </div><div> </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">
<br>
>><br>
>> In the meantime, review inline below.<br>
>><br>
>>> Index: include/llvm/ADT/APInt.h<br>
>>> ===================================================================<br>
>>> --- include/llvm/ADT/APInt.h<br>
>>> +++ include/llvm/ADT/APInt.h<br>
>>> @@ -1038,7 +1038,9 @@<br>
>>> /// the validity of the less-than relationship.<br>
>>> ///<br>
>>> /// \returns true if *this < RHS when considered unsigned.<br>
>>> - bool ult(uint64_t RHS) const { return ult(APInt(getBitWidth(), RHS)); }<br>
>>> + bool ult(uint64_t RHS) const {<br>
>>> + return getActiveBits() > 64 ? false : getZExtValue() < RHS;<br>
>>> + }<br>
>>> /// \brief Signed less than comparison<br>
>>> ///<br>
>>> @@ -1054,7 +1056,9 @@<br>
>>> /// the validity of the less-than relationship.<br>
>>> ///<br>
>>> /// \returns true if *this < RHS when considered signed.<br>
>>> - bool slt(uint64_t RHS) const { return slt(APInt(getBitWidth(), RHS)); }<br>
>>> + bool slt(int64_t RHS) const {<br>
>>> + return getMinSignedBits() >= 64 ? isNegative() : getSExtValue() < RHS;<br>
>> Shouldn't this be `> 64`?<br>
>><br>
>> Consider `this` equal to 0x8000000000000000:<br>
>> - getMinSignedBits() => 64<br>
>> - isNegative() => false<br>
>> - this->slt(0x8000000000000001) => false<br>
>> - this->slt(0x8000000000000000) => false<br>
>> - this->slt(0x7FFFFFFFFFFFFFFF) => false (??)<br>
>><br>
>>> + }<br>
>>> /// \brief Unsigned less or equal comparison<br>
>>> ///<br>
>>> @@ -1070,7 +1074,7 @@<br>
>>> /// the validity of the less-or-equal relationship.<br>
>>> ///<br>
>>> /// \returns true if *this <= RHS when considered unsigned.<br>
>>> - bool ule(uint64_t RHS) const { return ule(APInt(getBitWidth(), RHS)); }<br>
>>> + bool ule(uint64_t RHS) const { return ult(RHS) || *this == RHS; }<br>
>> It's kind of gross to be calling into `ult()` and `==`, since those are<br>
>> both non-trivial.<br>
>><br>
>> IMO, you should implement these directly:<br>
>> - sgt<br>
>> - slt<br>
>> - ugt<br>
>> - ult<br>
>><br>
>> And implement these trivially in terms of those:<br>
>> - sge<br>
>> - sle<br>
>> - uge<br>
>> - ule<br>
>><br>
>>> /// \brief Signed less or equal comparison<br>
>>> ///<br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>