[PATCH] Change APInt comparison with uint64_t.

[Duncan P. N. Exon Smith]

> On 2015 Jun 24, at 15:02, Paweł Bylica <chfast at gmail.com> wrote:
> 
> 
> 
> On Wed, Jun 24, 2015 at 7:04 PM Duncan P. N. Exon Smith <dexonsmith at apple.com> 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.
> 
> 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`?
> 
> You are right, it should bo.
>  
> Consider `this` equal to 0x8000000000000000:
>   - getMinSignedBits() => 64
>   - isNegative() => false
>   - this->slt(0x8000000000000001) => false
>   - this->slt(0x8000000000000000) => false
>   - this->slt(0x7FFFFFFFFFFFFFFF) => false (??)
> 
> However your example is wrong. 
> Consider `this` equal to 0x8000000000000000u == -0x8000000000000000:
>   - getMinSignedBits() => 64
>   - isNegative() => true
>   - this->slt(0x8000000000000001u) => this->slt(-0x7FFFFFFFFFFFFFFF) => true
>   - this->slt(0x8000000000000000u) => this->slt(-0x8000000000000000) => true (wrong!)
>   - this->slt(0x7FFFFFFFFFFFFFFFu) => true (wrong!)
> 
> I will add that do unit tests. Thanks.

Right.  I should have just used binary; my bin2hex converter is bad.
Sadly, what I meant was 0x400...0 (+/- 1).  Please test this too ;).

>  
> > +  }
> >
> >    /// \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
> 
> That might be a good idea. But because this patch can be controversial I would like not to include this optimization. The optimization should go with another patch even before this one. I would like also to measure the performance impact but I don't know how to do it. Is the test-suite the thing to use?
> 
> Another case is `ult` and `==` asymmetry. There is a method called `eq` but without overloading for uint64_t. Should I add APInt::eq(uint64_t)?
> 
> - PB