<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Jun 29, 2015 at 9:51 PM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On 2015-Jun-29, at 10:21, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
><br>
>><br>
>> On 2015-Jun-29, at 01:04, Paweł Bylica <<a href="mailto:chfast@gmail.com" target="_blank">chfast@gmail.com</a>> wrote:<br>
>><br>
>> 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>
>><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=n-sUoynO5gktRn-AS41_jaJts_UtutkDM8N77bJKJ14&s=4kY-0flQSlDMguMh951sV2bIOsPmyPuoSF8U3c_V8h8&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>
>><br>
>> I believe (2) is much more useful because of the following pattern can be found all over the code:<br>
>><br>
>> uint64_t BitWidth = getTypeSizeInBits(U->getType());<br>
>> if (CI->getValue().uge(BitWidth))<br>
>> break;<br>
>><br>
>> Sometimes also incorrectly expressed as CI->getZExtValue() >= BitWidth.<br>
>><br>
>> 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).<br>
>><br>
><br>
> Okay, good enough for me. LGTM once you make the test names useful.<br>
><br>
>> Index: unittests/ADT/APIntTest.cpp<br>
>> ===================================================================<br>
>> --- unittests/ADT/APIntTest.cpp<br>
>> +++ unittests/ADT/APIntTest.cpp<br>
>> @@ -216,7 +216,7 @@<br>
>> }<br>
>> }<br>
>><br>
>> -TEST(APIntTest, compare) {<br>
>> +TEST(APIntTest, compare1) {<br>
><br>
> I don't think you need to change this name.<br>
><br>
>> std::array<APInt, 5> testVals{{<br>
>> APInt{16, 2},<br>
>> APInt{16, 1},<br>
>> @@ -254,6 +254,133 @@<br>
>> }<br>
>> }<br>
>><br>
>> +TEST(APIntTest, compare2) {<br>
><br>
> IMO, you should name this by whatever theme you chose to group these<br>
> together. Maybe "compareWithRawIntegers"? Something more descriptive<br>
> than `compare2`, anyway. Maybe you even want to break it up slightly<br>
> so that you can come up with good names. Up to you.<br>
><br>
>> + EXPECT_TRUE(!APInt(8, 1).uge(256));<br>
>> + EXPECT_TRUE(!APInt(8, 1).ugt(256));<br>
>> + EXPECT_TRUE( APInt(8, 1).ule(256));<br>
>> + EXPECT_TRUE( APInt(8, 1).ult(256));<br>
>> + EXPECT_TRUE(!APInt(8, 1).sge(256));<br>
>> + EXPECT_TRUE(!APInt(8, 1).sgt(256));<br>
>> + EXPECT_TRUE( APInt(8, 1).sle(256));<br>
>> + EXPECT_TRUE( APInt(8, 1).slt(256));<br>
>> + EXPECT_TRUE(!(APInt(8, 0) == 256));<br>
>> + EXPECT_TRUE( APInt(8, 0) != 256);<br>
>> + EXPECT_TRUE(!(APInt(8, 1) == 256));<br>
>> + EXPECT_TRUE( APInt(8, 1) != 256);<br>
>> +<br>
>> + auto uint64max = std::numeric_limits<uint64_t>::max();<br>
>> + auto int64max = std::numeric_limits<int64_t>::max();<br>
>> + auto int64min = std::numeric_limits<int64_t>::min();<br>
>> +<br>
>> + auto u64 = APInt{128, uint64max};<br>
>> + auto s64 = APInt{128, static_cast<uint64_t>(int64max), true};<br>
>> + auto big = u64 + 1;<br>
>> +<br>
>> + EXPECT_TRUE( u64.uge(uint64max));<br>
>> + EXPECT_TRUE(!u64.ugt(uint64max));<br>
>> + EXPECT_TRUE( u64.ule(uint64max));<br>
>> + EXPECT_TRUE(!u64.ult(uint64max));<br>
>> + EXPECT_TRUE( u64.sge(int64max));<br>
>> + EXPECT_TRUE( u64.sgt(int64max));<br>
>> + EXPECT_TRUE(!u64.sle(int64max));<br>
>> + EXPECT_TRUE(!u64.slt(int64max));<br>
>> + EXPECT_TRUE( u64.sge(int64min));<br>
>> + EXPECT_TRUE( u64.sgt(int64min));<br>
>> + EXPECT_TRUE(!u64.sle(int64min));<br>
>> + EXPECT_TRUE(!u64.slt(int64min));<br>
>> +<br>
>> + EXPECT_TRUE(u64 == uint64max);<br>
>> + EXPECT_TRUE(u64 != int64max);<br>
>> + EXPECT_TRUE(u64 != int64min);<br>
>> +<br>
>> + EXPECT_TRUE(!s64.uge(uint64max));<br>
>> + EXPECT_TRUE(!s64.ugt(uint64max));<br>
>> + EXPECT_TRUE( s64.ule(uint64max));<br>
>> + EXPECT_TRUE( s64.ult(uint64max));<br>
>> + EXPECT_TRUE( s64.sge(int64max));<br>
>> + EXPECT_TRUE(!s64.sgt(int64max));<br>
>> + EXPECT_TRUE( s64.sle(int64max));<br>
>> + EXPECT_TRUE(!s64.slt(int64max));<br>
>> + EXPECT_TRUE( s64.sge(int64min));<br>
>> + EXPECT_TRUE( s64.sgt(int64min));<br>
>> + EXPECT_TRUE(!s64.sle(int64min));<br>
>> + EXPECT_TRUE(!s64.slt(int64min));<br>
>> +<br>
>> + EXPECT_TRUE(s64 != uint64max);<br>
>> + EXPECT_TRUE(s64 == int64max);<br>
>> + EXPECT_TRUE(s64 != int64min);<br>
>> +<br>
>> + EXPECT_TRUE( big.uge(uint64max));<br>
>> + EXPECT_TRUE( big.ugt(uint64max));<br>
>> + EXPECT_TRUE(!big.ule(uint64max));<br>
>> + EXPECT_TRUE(!big.ult(uint64max));<br>
>> + EXPECT_TRUE( big.sge(int64max));<br>
>> + EXPECT_TRUE( big.sgt(int64max));<br>
>> + EXPECT_TRUE(!big.sle(int64max));<br>
>> + EXPECT_TRUE(!big.slt(int64max));<br>
>> + EXPECT_TRUE( big.sge(int64min));<br>
>> + EXPECT_TRUE( big.sgt(int64min));<br>
>> + EXPECT_TRUE(!big.sle(int64min));<br>
>> + EXPECT_TRUE(!big.slt(int64min));<br>
>> +<br>
>> + EXPECT_TRUE(big != uint64max);<br>
>> + EXPECT_TRUE(big != int64max);<br>
>> + EXPECT_TRUE(big != int64min);<br>
>> +}<br>
>> +<br>
>> +TEST(APIntTest, compare3) {<br>
><br>
> Maybe "compareWithIntMin()"?<br>
><br>
>> + int64_t edge = -0x8000000000000000;<br>
<br>
(And you should just use `INT64_MIN` here.)<br>
<br>
>> + int64_t edgeP1 = edge + 1;<br>
>> + int64_t edgeM1 = edge - 1;<br>
<br>
Hmm, looking more closely you have UB here. Subtracting 1 from `INT64_MIN`<br>
isn't legal. You should fix that before you commit (just use `INT64_MAX`?).<br></blockquote><div><br></div><div>You're right. I just ignored the UB.</div><div>Is INT64_MAX prefered over std::numeric_limit? I dislike INT64_MAX because it required __STDC_LIMIT_MACROS to be defined. That requirements propagates also to projects that use LLVM.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
>> + auto a = APInt{64, static_cast<uint64_t>(edge), true};<br>
>> +<br>
>> + EXPECT_TRUE(!a.slt(edge));<br>
>> + EXPECT_TRUE( a.sle(edge));<br>
>> + EXPECT_TRUE(!a.sgt(edge));<br>
>> + EXPECT_TRUE( a.sge(edge));<br>
>> + EXPECT_TRUE( a.slt(edgeP1));<br>
>> + EXPECT_TRUE( a.sle(edgeP1));<br>
>> + EXPECT_TRUE(!a.sgt(edgeP1));<br>
>> + EXPECT_TRUE(!a.sge(edgeP1));<br>
>> + EXPECT_TRUE( a.slt(edgeM1));<br>
>> + EXPECT_TRUE( a.sle(edgeM1));<br>
>> + EXPECT_TRUE(!a.sgt(edgeM1));<br>
>> + EXPECT_TRUE(!a.sge(edgeM1));<br>
>> +}<br>
>> +<br>
>> +TEST(APIntTest, compare4) {<br>
><br>
> Maybe "compareWithLargeInt"?<br>
><br>
>> + uint64_t edge = 0x4000000000000000;<br>
>> + uint64_t edgeP1 = edge + 1;<br>
>> + uint64_t edgeM1 = edge - 1;<br>
>> + auto a = APInt{64, edge};<br>
>> +<br>
>> + EXPECT_TRUE(!a.ult(edge));<br>
>> + EXPECT_TRUE( a.ule(edge));<br>
>> + EXPECT_TRUE(!a.ugt(edge));<br>
>> + EXPECT_TRUE( a.uge(edge));<br>
>> + EXPECT_TRUE( a.ult(edgeP1));<br>
>> + EXPECT_TRUE( a.ule(edgeP1));<br>
>> + EXPECT_TRUE(!a.ugt(edgeP1));<br>
>> + EXPECT_TRUE(!a.uge(edgeP1));<br>
>> + EXPECT_TRUE(!a.ult(edgeM1));<br>
>> + EXPECT_TRUE(!a.ule(edgeM1));<br>
>> + EXPECT_TRUE( a.ugt(edgeM1));<br>
>> + EXPECT_TRUE( a.uge(edgeM1));<br>
>> +<br>
>> + EXPECT_TRUE(!a.slt(edge));<br>
>> + EXPECT_TRUE( a.sle(edge));<br>
>> + EXPECT_TRUE(!a.sgt(edge));<br>
>> + EXPECT_TRUE( a.sge(edge));<br>
>> + EXPECT_TRUE( a.slt(edgeP1));<br>
>> + EXPECT_TRUE( a.sle(edgeP1));<br>
>> + EXPECT_TRUE(!a.sgt(edgeP1));<br>
>> + EXPECT_TRUE(!a.sge(edgeP1));<br>
>> + EXPECT_TRUE(!a.slt(edgeM1));<br>
>> + EXPECT_TRUE(!a.sle(edgeM1));<br>
>> + EXPECT_TRUE( a.sgt(edgeM1));<br>
>> + EXPECT_TRUE( a.sge(edgeM1));<br>
>> +}<br>
>> +<br>
>><br>
>> // Tests different div/rem varaints using scheme (a * b + c) / a<br>
>> void testDiv(APInt a, APInt b, APInt c) {<br>
>><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>
</blockquote></div></div>