[PATCH] Change APInt comparison with uint64_t.
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Jun 29 12:51:36 PDT 2015
> On 2015-Jun-29, at 10:21, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
>>
>> On 2015-Jun-29, at 01:04, Paweł Bylica <chfast at gmail.com> wrote:
>>
>> On Sat, Jun 27, 2015 at 2:49 AM Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>
>>> On 2015-Jun-26, at 11:42, Philip Reames <listmail at philipreames.com> wrote:
>>>
>>>
>>>
>>> On 06/24/2015 10:04 AM, Duncan P. N. Exon Smith wrote:
>>>>> On 2015 Jun 23, at 08:14, Paweł Bylica <chfast at gmail.com> wrote:
>>>>>
>>>>> Hi chandlerc,
>>>>>
>>>>> This patch changes the way APInt is compared with a value of type uint64_t.
>>>>> Before the uint64_t value was truncated to the size of APInt before comparison.
>>>>> Now the comparison takes into account full 64-bit precision.
>>>>>
>>>>> http://reviews.llvm.org/D10655
>>>>>
>>>>> Files:
>>>>> include/llvm/ADT/APInt.h
>>>>> unittests/ADT/APIntTest.cpp
>>>>>
>>>> You never got a response from your llvmdev post. There are two ways to
>>>> go here:
>>>>
>>>> 1. Assert that the value is in the range of BitWidth.
>>>> 2. Extend this to 64-bits and compare.
>>>>
>>>> I'm inclined to agree that (2) is more useful -- developers can opt-in
>>>> to the old behaviour by hand-constructing an `APInt()` -- but I'd like
>>>> to hear from someone else before this is committed.
>>> 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.
>>
>> Anyone else? Pawel, any particular reason you didn't go for (1)?
>>
>> I believe (2) is much more useful because of the following pattern can be found all over the code:
>>
>> uint64_t BitWidth = getTypeSizeInBits(U->getType());
>> if (CI->getValue().uge(BitWidth))
>> break;
>>
>> Sometimes also incorrectly expressed as CI->getZExtValue() >= BitWidth.
>>
>> 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).
>>
>
> Okay, good enough for me. LGTM once you make the test names useful.
>
>> Index: unittests/ADT/APIntTest.cpp
>> ===================================================================
>> --- unittests/ADT/APIntTest.cpp
>> +++ unittests/ADT/APIntTest.cpp
>> @@ -216,7 +216,7 @@
>> }
>> }
>>
>> -TEST(APIntTest, compare) {
>> +TEST(APIntTest, compare1) {
>
> I don't think you need to change this name.
>
>> std::array<APInt, 5> testVals{{
>> APInt{16, 2},
>> APInt{16, 1},
>> @@ -254,6 +254,133 @@
>> }
>> }
>>
>> +TEST(APIntTest, compare2) {
>
> IMO, you should name this by whatever theme you chose to group these
> together. Maybe "compareWithRawIntegers"? Something more descriptive
> than `compare2`, anyway. Maybe you even want to break it up slightly
> so that you can come up with good names. Up to you.
>
>> + EXPECT_TRUE(!APInt(8, 1).uge(256));
>> + EXPECT_TRUE(!APInt(8, 1).ugt(256));
>> + EXPECT_TRUE( APInt(8, 1).ule(256));
>> + EXPECT_TRUE( APInt(8, 1).ult(256));
>> + EXPECT_TRUE(!APInt(8, 1).sge(256));
>> + EXPECT_TRUE(!APInt(8, 1).sgt(256));
>> + EXPECT_TRUE( APInt(8, 1).sle(256));
>> + EXPECT_TRUE( APInt(8, 1).slt(256));
>> + EXPECT_TRUE(!(APInt(8, 0) == 256));
>> + EXPECT_TRUE( APInt(8, 0) != 256);
>> + EXPECT_TRUE(!(APInt(8, 1) == 256));
>> + EXPECT_TRUE( APInt(8, 1) != 256);
>> +
>> + auto uint64max = std::numeric_limits<uint64_t>::max();
>> + auto int64max = std::numeric_limits<int64_t>::max();
>> + auto int64min = std::numeric_limits<int64_t>::min();
>> +
>> + auto u64 = APInt{128, uint64max};
>> + auto s64 = APInt{128, static_cast<uint64_t>(int64max), true};
>> + auto big = u64 + 1;
>> +
>> + EXPECT_TRUE( u64.uge(uint64max));
>> + EXPECT_TRUE(!u64.ugt(uint64max));
>> + EXPECT_TRUE( u64.ule(uint64max));
>> + EXPECT_TRUE(!u64.ult(uint64max));
>> + EXPECT_TRUE( u64.sge(int64max));
>> + EXPECT_TRUE( u64.sgt(int64max));
>> + EXPECT_TRUE(!u64.sle(int64max));
>> + EXPECT_TRUE(!u64.slt(int64max));
>> + EXPECT_TRUE( u64.sge(int64min));
>> + EXPECT_TRUE( u64.sgt(int64min));
>> + EXPECT_TRUE(!u64.sle(int64min));
>> + EXPECT_TRUE(!u64.slt(int64min));
>> +
>> + EXPECT_TRUE(u64 == uint64max);
>> + EXPECT_TRUE(u64 != int64max);
>> + EXPECT_TRUE(u64 != int64min);
>> +
>> + EXPECT_TRUE(!s64.uge(uint64max));
>> + EXPECT_TRUE(!s64.ugt(uint64max));
>> + EXPECT_TRUE( s64.ule(uint64max));
>> + EXPECT_TRUE( s64.ult(uint64max));
>> + EXPECT_TRUE( s64.sge(int64max));
>> + EXPECT_TRUE(!s64.sgt(int64max));
>> + EXPECT_TRUE( s64.sle(int64max));
>> + EXPECT_TRUE(!s64.slt(int64max));
>> + EXPECT_TRUE( s64.sge(int64min));
>> + EXPECT_TRUE( s64.sgt(int64min));
>> + EXPECT_TRUE(!s64.sle(int64min));
>> + EXPECT_TRUE(!s64.slt(int64min));
>> +
>> + EXPECT_TRUE(s64 != uint64max);
>> + EXPECT_TRUE(s64 == int64max);
>> + EXPECT_TRUE(s64 != int64min);
>> +
>> + EXPECT_TRUE( big.uge(uint64max));
>> + EXPECT_TRUE( big.ugt(uint64max));
>> + EXPECT_TRUE(!big.ule(uint64max));
>> + EXPECT_TRUE(!big.ult(uint64max));
>> + EXPECT_TRUE( big.sge(int64max));
>> + EXPECT_TRUE( big.sgt(int64max));
>> + EXPECT_TRUE(!big.sle(int64max));
>> + EXPECT_TRUE(!big.slt(int64max));
>> + EXPECT_TRUE( big.sge(int64min));
>> + EXPECT_TRUE( big.sgt(int64min));
>> + EXPECT_TRUE(!big.sle(int64min));
>> + EXPECT_TRUE(!big.slt(int64min));
>> +
>> + EXPECT_TRUE(big != uint64max);
>> + EXPECT_TRUE(big != int64max);
>> + EXPECT_TRUE(big != int64min);
>> +}
>> +
>> +TEST(APIntTest, compare3) {
>
> Maybe "compareWithIntMin()"?
>
>> + int64_t edge = -0x8000000000000000;
(And you should just use `INT64_MIN` here.)
>> + int64_t edgeP1 = edge + 1;
>> + int64_t edgeM1 = edge - 1;
Hmm, looking more closely you have UB here. Subtracting 1 from `INT64_MIN`
isn't legal. You should fix that before you commit (just use `INT64_MAX`?).
>> + auto a = APInt{64, static_cast<uint64_t>(edge), true};
>> +
>> + EXPECT_TRUE(!a.slt(edge));
>> + EXPECT_TRUE( a.sle(edge));
>> + EXPECT_TRUE(!a.sgt(edge));
>> + EXPECT_TRUE( a.sge(edge));
>> + EXPECT_TRUE( a.slt(edgeP1));
>> + EXPECT_TRUE( a.sle(edgeP1));
>> + EXPECT_TRUE(!a.sgt(edgeP1));
>> + EXPECT_TRUE(!a.sge(edgeP1));
>> + EXPECT_TRUE( a.slt(edgeM1));
>> + EXPECT_TRUE( a.sle(edgeM1));
>> + EXPECT_TRUE(!a.sgt(edgeM1));
>> + EXPECT_TRUE(!a.sge(edgeM1));
>> +}
>> +
>> +TEST(APIntTest, compare4) {
>
> Maybe "compareWithLargeInt"?
>
>> + uint64_t edge = 0x4000000000000000;
>> + uint64_t edgeP1 = edge + 1;
>> + uint64_t edgeM1 = edge - 1;
>> + auto a = APInt{64, edge};
>> +
>> + EXPECT_TRUE(!a.ult(edge));
>> + EXPECT_TRUE( a.ule(edge));
>> + EXPECT_TRUE(!a.ugt(edge));
>> + EXPECT_TRUE( a.uge(edge));
>> + EXPECT_TRUE( a.ult(edgeP1));
>> + EXPECT_TRUE( a.ule(edgeP1));
>> + EXPECT_TRUE(!a.ugt(edgeP1));
>> + EXPECT_TRUE(!a.uge(edgeP1));
>> + EXPECT_TRUE(!a.ult(edgeM1));
>> + EXPECT_TRUE(!a.ule(edgeM1));
>> + EXPECT_TRUE( a.ugt(edgeM1));
>> + EXPECT_TRUE( a.uge(edgeM1));
>> +
>> + EXPECT_TRUE(!a.slt(edge));
>> + EXPECT_TRUE( a.sle(edge));
>> + EXPECT_TRUE(!a.sgt(edge));
>> + EXPECT_TRUE( a.sge(edge));
>> + EXPECT_TRUE( a.slt(edgeP1));
>> + EXPECT_TRUE( a.sle(edgeP1));
>> + EXPECT_TRUE(!a.sgt(edgeP1));
>> + EXPECT_TRUE(!a.sge(edgeP1));
>> + EXPECT_TRUE(!a.slt(edgeM1));
>> + EXPECT_TRUE(!a.sle(edgeM1));
>> + EXPECT_TRUE( a.sgt(edgeM1));
>> + EXPECT_TRUE( a.sge(edgeM1));
>> +}
>> +
>>
>> // Tests different div/rem varaints using scheme (a * b + c) / a
>> void testDiv(APInt a, APInt b, APInt c) {
>>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list