[PATCH] Change APInt comparison with uint64_t.
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Jun 29 10:21:10 PDT 2015
> 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;
> + int64_t edgeP1 = edge + 1;
> + int64_t edgeM1 = edge - 1;
> + 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) {
>
More information about the llvm-commits
mailing list