[PATCH] Change APInt comparison with uint64_t.
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Jun 29 15:41:51 PDT 2015
> On 2015-Jun-29, at 13:11, Paweł Bylica <chfast at gmail.com> wrote:
>
> 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.
>
LLVM uses (e.g.) INT64_MAX and INT64_C(234) pervasively. The build
system defines the necessary macros:
--
$ git grep __STDC_LIMIT_MACROS
Makefile.rules:CPP.BaseFlags += -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
cmake/modules/HandleLLVMOptions.cmake:add_llvm_definitions( -D__STDC_LIMIT_MACROS )
cmake/modules/LLVMConfig.cmake.in:set(LLVM_DEFINITIONS "-D__STDC_LIMIT_MACROS" "-D__STDC_CONSTANT_MACROS")
include/llvm/Support/DataTypes.h.cmake:/* Note that this header's correct operation depends on __STDC_LIMIT_MACROS
include/llvm/Support/DataTypes.h.cmake:#if !defined(__STDC_LIMIT_MACROS)
include/llvm/Support/DataTypes.h.cmake:# error "Must #define __STDC_LIMIT_MACROS before #including Support/DataTypes.h"
include/llvm/Support/DataTypes.h.in:/* Note that this header's correct operation depends on __STDC_LIMIT_MACROS
include/llvm/Support/DataTypes.h.in:#if !defined(__STDC_LIMIT_MACROS)
include/llvm/Support/DataTypes.h.in:# error "Must #define __STDC_LIMIT_MACROS before #including Support/DataTypes.h"
utils/vim/vimrc: \ "-D__STDC_LIMIT_MACROS=1","-D__STDC_CONSTANT_MACROS=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) {
> >>
> >
> >
> >
> > _______________________________________________
> > 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