[PATCH] Change APInt comparison with uint64_t.

[Duncan P. N. Exon Smith]

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

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