[PATCH] Change APInt comparison with uint64_t.

[Philip Reames]

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