[PATCH] Change APInt comparison with uint64_t.

[Paweł Bylica]

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).


>
> >>
> >> In the meantime, review inline below.
> >>
> >>> Index: include/llvm/ADT/APInt.h
> >>> ===================================================================
> >>> --- include/llvm/ADT/APInt.h
> >>> +++ include/llvm/ADT/APInt.h
> >>> @@ -1038,7 +1038,9 @@
> >>>    /// the validity of the less-than relationship.
> >>>    ///
> >>>    /// \returns true if *this < RHS when considered unsigned.
> >>> -  bool ult(uint64_t RHS) const { return ult(APInt(getBitWidth(),
> RHS)); }
> >>> +  bool ult(uint64_t RHS) const {
> >>> +    return getActiveBits() > 64 ? false : getZExtValue() < RHS;
> >>> +  }
> >>>      /// \brief Signed less than comparison
> >>>    ///
> >>> @@ -1054,7 +1056,9 @@
> >>>    /// the validity of the less-than relationship.
> >>>    ///
> >>>    /// \returns true if *this < RHS when considered signed.
> >>> -  bool slt(uint64_t RHS) const { return slt(APInt(getBitWidth(),
> RHS)); }
> >>> +  bool slt(int64_t RHS) const {
> >>> +    return getMinSignedBits() >= 64 ? isNegative() : getSExtValue() <
> RHS;
> >> Shouldn't this be `> 64`?
> >>
> >> Consider `this` equal to 0x8000000000000000:
> >>   - getMinSignedBits() => 64
> >>   - isNegative() => false
> >>   - this->slt(0x8000000000000001) => false
> >>   - this->slt(0x8000000000000000) => false
> >>   - this->slt(0x7FFFFFFFFFFFFFFF) => false (??)
> >>
> >>> +  }
> >>>      /// \brief Unsigned less or equal comparison
> >>>    ///
> >>> @@ -1070,7 +1074,7 @@
> >>>    /// the validity of the less-or-equal relationship.
> >>>    ///
> >>>    /// \returns true if *this <= RHS when considered unsigned.
> >>> -  bool ule(uint64_t RHS) const { return ule(APInt(getBitWidth(),
> RHS)); }
> >>> +  bool ule(uint64_t RHS) const { return ult(RHS) || *this == RHS; }
> >> It's kind of gross to be calling into `ult()` and `==`, since those are
> >> both non-trivial.
> >>
> >> IMO, you should implement these directly:
> >>   - sgt
> >>   - slt
> >>   - ugt
> >>   - ult
> >>
> >> And implement these trivially in terms of those:
> >>   - sge
> >>   - sle
> >>   - uge
> >>   - ule
> >>
> >>>      /// \brief Signed less or equal comparison
> >>>    ///
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> _______________________________________________
> 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/f3df31e5/attachment.html>