[PATCH] Change APInt comparison with uint64_t.
Paweł Bylica
chfast at gmail.com
Mon Jun 29 13:11:40 PDT 2015
On Mon, Jun 29, 2015 at 9:51 PM Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > 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`?).
>
You're right. I just ignored the UB.
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.
>
> >> + 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150629/99035dc4/attachment.html>
More information about the llvm-commits
mailing list