<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jun 24, 2015 at 7:04 PM 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 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=hPnB295ntn3pfYGYVW_ouvBVBJy_IRyzeHs-TbgkW6w&s=_S7CXemIMPd-z1JPKZrqH6PHV_A-_nFR7hkymHfWJPk&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>
<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>
<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>
><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>
<br>
Shouldn't this be `> 64`?<br></blockquote><div><br></div><div>You are right, it should bo.</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">
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></blockquote><div><br></div><div>However your example is wrong. </div><div>Consider `this` equal to 0x8000000000000000u == -0x8000000000000000:</div><div> - getMinSignedBits() => 64<br> - isNegative() => true<br> - this->slt(0x8000000000000001u) => this->slt(-0x7FFFFFFFFFFFFFFF) => true</div><div> - this->slt(0x8000000000000000u) => this->slt(-0x8000000000000000) => true (wrong!)<br> - this->slt(0x7FFFFFFFFFFFFFFFu) => true (wrong!)<br></div><div><br></div><div>I will add that do unit tests. Thanks.</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>
> /// \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>
<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></blockquote><div><br></div><div>That might be a good idea. But because this patch can be controversial I would like not to include this optimization. The optimization should go with another patch even before this one. I would like also to measure the performance impact but I don't know how to do it. Is the test-suite the thing to use?</div><div><br></div><div>Another case is `ult` and `==` asymmetry. There is a method called `eq` but without overloading for uint64_t. Should I add APInt::eq(uint64_t)?</div><div><br></div><div>- PB</div></div></div>