[PATCH] Add APInt(numBits, ArrayRef<uint64_t> bigVal) constructor

Add APInt(numBits, ArrayRef<uint64_t> bigVal) constructor to prevent future ambiguity errors like the one corrected by r135261.  Migrate all LLVM callers of the old constructor to the new one.

[Jeffrey Yasskin, 2011-07-17 05:51:56]

Initial diff at http://codereview.appspot.com/download/issue4749046_1.diff

[Jeffrey Yasskin (google), 2011-07-18 16:58:04]

Here's the patch.

On Sat, Jul 16, 2011 at 10:51 PM,  <jyasskin@gmail.com> wrote:
> Reviewers: eli.friedman_gmail.com,
>
> Message:
> Initial diff at
> http://codereview.appspot.com/download/issue4749046_1.diff
>
> Description:
> Add APInt(numBits, ArrayRef<uint64_t> bigVal) constructor to prevent
> future ambiguity errors like the one corrected by r135261.  Migrate all
> LLVM callers of the old constructor to the new one.
>
>
> Please review this at http://codereview.appspot.com/4749046/
>
> Affected files:
>   M include/llvm/ADT/APInt.h
>   M lib/AsmParser/LLLexer.cpp
>   M lib/Bitcode/Reader/BitcodeReader.cpp
>   M lib/CodeGen/SelectionDAG/FastISel.cpp
>   M lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
>   M lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>   M lib/ExecutionEngine/ExecutionEngine.cpp
>   M lib/Support/APFloat.cpp
>   M lib/Support/APInt.cpp
>   M lib/VMCore/ConstantFold.cpp
>   M lib/VMCore/Core.cpp
>   M unittests/ADT/APIntTest.cpp
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>

[eli.friedman_gmail.com, 2011-07-18 18:33:17]

On Mon, Jul 18, 2011 at 9:57 AM, Jeffrey Yasskin <jyasskin@google.com> wrote:
> Here's the patch.
>
> On Sat, Jul 16, 2011 at 10:51 PM,  <jyasskin@gmail.com> wrote:
>> Reviewers: eli.friedman_gmail.com,
>>
>> Message:
>> Initial diff at
>> http://codereview.appspot.com/download/issue4749046_1.diff
>>
>> Description:
>> Add APInt(numBits, ArrayRef<uint64_t> bigVal) constructor to prevent
>> future ambiguity errors like the one corrected by r135261.  Migrate all
>> LLVM callers of the old constructor to the new one.
>>
>>
>> Please review this at http://codereview.appspot.com/4749046/
>>
>> Affected files:
>>   M include/llvm/ADT/APInt.h
>>   M lib/AsmParser/LLLexer.cpp
>>   M lib/Bitcode/Reader/BitcodeReader.cpp
>>   M lib/CodeGen/SelectionDAG/FastISel.cpp
>>   M lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
>>   M lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>>   M lib/ExecutionEngine/ExecutionEngine.cpp
>>   M lib/Support/APFloat.cpp
>>   M lib/Support/APInt.cpp
>>   M lib/VMCore/ConstantFold.cpp
>>   M lib/VMCore/Core.cpp
>>   M unittests/ADT/APIntTest.cpp

@@ -342,7 +353,8 @@ public:

     if (isSingleWord())
       return isUIntN(N, VAL);
-    return APInt(N, getNumWords(), pVal).zext(getBitWidth()) == (*this);
+    return APInt(N, ArrayRef<uint64_t>(pVal,
getNumWords())).zext(getBitWidth())
+      == (*this);
   }

   /// @brief Check if this APInt has an N-bits signed integer value.

makeArrayRef just landed; use it. :)  Same in a couple other places.

Otherwise, looks fine.

As a followup, could you eliminate both the "APInt(unsigned numBits,
unsigned numWords, const uint64_t bigVal[])" and the "APInt(unsigned
numBits, uint64_t val, bool isSigned = false)" constructors?  (It
should just require ArrayRef-izing and adding an
"APInt::getSigned(unsigned numBits, uint64_t val)" helper.)

-Eli

[Jeffrey Yasskin (google), 2011-07-18 18:42:56]

On Mon, Jul 18, 2011 at 11:33 AM, Eli Friedman <eli.friedman@gmail.com> wrote:
> On Mon, Jul 18, 2011 at 9:57 AM, Jeffrey Yasskin <jyasskin@google.com> wrote:
>> Here's the patch.
>>
>> On Sat, Jul 16, 2011 at 10:51 PM,  <jyasskin@gmail.com> wrote:
>>> Reviewers: eli.friedman_gmail.com,
>>>
>>> Message:
>>> Initial diff at
>>> http://codereview.appspot.com/download/issue4749046_1.diff
>>>
>>> Description:
>>> Add APInt(numBits, ArrayRef<uint64_t> bigVal) constructor to prevent
>>> future ambiguity errors like the one corrected by r135261.  Migrate all
>>> LLVM callers of the old constructor to the new one.
>>>
>>>
>>> Please review this at http://codereview.appspot.com/4749046/
>>>
>>> Affected files:
>>>   M include/llvm/ADT/APInt.h
>>>   M lib/AsmParser/LLLexer.cpp
>>>   M lib/Bitcode/Reader/BitcodeReader.cpp
>>>   M lib/CodeGen/SelectionDAG/FastISel.cpp
>>>   M lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
>>>   M lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>>>   M lib/ExecutionEngine/ExecutionEngine.cpp
>>>   M lib/Support/APFloat.cpp
>>>   M lib/Support/APInt.cpp
>>>   M lib/VMCore/ConstantFold.cpp
>>>   M lib/VMCore/Core.cpp
>>>   M unittests/ADT/APIntTest.cpp
>
> @@ -342,7 +353,8 @@ public:
>
>     if (isSingleWord())
>       return isUIntN(N, VAL);
> -    return APInt(N, getNumWords(), pVal).zext(getBitWidth()) == (*this);
> +    return APInt(N, ArrayRef<uint64_t>(pVal,
> getNumWords())).zext(getBitWidth())
> +      == (*this);
>   }
>
>   /// @brief Check if this APInt has an N-bits signed integer value.
>
> makeArrayRef just landed; use it. :)  Same in a couple other places.

Silly codebase, changing out from under me. ;-) Will do.

> Otherwise, looks fine.
>
> As a followup, could you eliminate both the "APInt(unsigned numBits,
> unsigned numWords, const uint64_t bigVal[])" and the "APInt(unsigned
> numBits, uint64_t val, bool isSigned = false)" constructors?  (It
> should just require ArrayRef-izing and adding an
> "APInt::getSigned(unsigned numBits, uint64_t val)" helper.)

How do you want to handle dependent codebases? I was thinking to leave
APInt(...bigVal[]) in for a version to let people switch, but I guess
LLVM does tend to remove things more aggressively than that. I'm only
willing to commit to fixing clang myself if you want me to both fix
things and remove the deprecated constructors.

[Jeffrey Yasskin (google), 2011-07-18 21:21:05]

On Mon, Jul 18, 2011 at 11:42 AM, Jeffrey Yasskin <jyasskin@google.com> wrote:
> On Mon, Jul 18, 2011 at 11:33 AM, Eli Friedman <eli.friedman@gmail.com> wrote:
>> On Mon, Jul 18, 2011 at 9:57 AM, Jeffrey Yasskin <jyasskin@google.com> wrote:
>>> Here's the patch.
>>>
>>> On Sat, Jul 16, 2011 at 10:51 PM,  <jyasskin@gmail.com> wrote:
>>>> Reviewers: eli.friedman_gmail.com,
>>>>
>>>> Message:
>>>> Initial diff at
>>>> http://codereview.appspot.com/download/issue4749046_1.diff
>>>>
>>>> Description:
>>>> Add APInt(numBits, ArrayRef<uint64_t> bigVal) constructor to prevent
>>>> future ambiguity errors like the one corrected by r135261.  Migrate all
>>>> LLVM callers of the old constructor to the new one.
>>>>
>>>>
>>>> Please review this at http://codereview.appspot.com/4749046/
>>>>
>>>> Affected files:
>>>>   M include/llvm/ADT/APInt.h
>>>>   M lib/AsmParser/LLLexer.cpp
>>>>   M lib/Bitcode/Reader/BitcodeReader.cpp
>>>>   M lib/CodeGen/SelectionDAG/FastISel.cpp
>>>>   M lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
>>>>   M lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>>>>   M lib/ExecutionEngine/ExecutionEngine.cpp
>>>>   M lib/Support/APFloat.cpp
>>>>   M lib/Support/APInt.cpp
>>>>   M lib/VMCore/ConstantFold.cpp
>>>>   M lib/VMCore/Core.cpp
>>>>   M unittests/ADT/APIntTest.cpp
>>
>> @@ -342,7 +353,8 @@ public:
>>
>>     if (isSingleWord())
>>       return isUIntN(N, VAL);
>> -    return APInt(N, getNumWords(), pVal).zext(getBitWidth()) == (*this);
>> +    return APInt(N, ArrayRef<uint64_t>(pVal,
>> getNumWords())).zext(getBitWidth())
>> +      == (*this);
>>   }
>>
>>   /// @brief Check if this APInt has an N-bits signed integer value.
>>
>> makeArrayRef just landed; use it. :)  Same in a couple other places.
>
> Silly codebase, changing out from under me. ;-) Will do.
>
>> Otherwise, looks fine.
>>
>> As a followup, could you eliminate both the "APInt(unsigned numBits,
>> unsigned numWords, const uint64_t bigVal[])" and the "APInt(unsigned
>> numBits, uint64_t val, bool isSigned = false)" constructors?  (It
>> should just require ArrayRef-izing and adding an
>> "APInt::getSigned(unsigned numBits, uint64_t val)" helper.)
>
> How do you want to handle dependent codebases? I was thinking to leave
> APInt(...bigVal[]) in for a version to let people switch, but I guess
> LLVM does tend to remove things more aggressively than that. I'm only
> willing to commit to fixing clang myself if you want me to both fix
> things and remove the deprecated constructors.

By which I mean that I'm not volunteering to fix anything besides
clang, but I am happy to fix clang.

Also, do you want APInt::getSigned() to take a uint64_t or an int64_t?
IIRC, taking uint64_t introduces some warnings when we pass literal
negative numbers, and we made ConstantInt::getSigned() take an int64_t
for that reason. Hm, without the bool-taking version of APInt(),
ConstantInt::get() will need an if, or we'll have to change it to not
accept signed numbers either.

[eli.friedman_gmail.com, 2011-07-18 22:00:10]

On Mon, Jul 18, 2011 at 2:20 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
> On Mon, Jul 18, 2011 at 11:42 AM, Jeffrey Yasskin <jyasskin@google.com> wrote:
>> On Mon, Jul 18, 2011 at 11:33 AM, Eli Friedman <eli.friedman@gmail.com>
wrote:
>>> On Mon, Jul 18, 2011 at 9:57 AM, Jeffrey Yasskin <jyasskin@google.com>
wrote:
>>>> Here's the patch.
>>>>
>>>> On Sat, Jul 16, 2011 at 10:51 PM,  <jyasskin@gmail.com> wrote:
>>>>> Reviewers: eli.friedman_gmail.com,
>>>>>
>>>>> Message:
>>>>> Initial diff at
>>>>> http://codereview.appspot.com/download/issue4749046_1.diff
>>>>>
>>>>> Description:
>>>>> Add APInt(numBits, ArrayRef<uint64_t> bigVal) constructor to prevent
>>>>> future ambiguity errors like the one corrected by r135261.  Migrate all
>>>>> LLVM callers of the old constructor to the new one.
>>>>>
>>>>>
>>>>> Please review this at http://codereview.appspot.com/4749046/
>>>>>
>>>>> Affected files:
>>>>>   M include/llvm/ADT/APInt.h
>>>>>   M lib/AsmParser/LLLexer.cpp
>>>>>   M lib/Bitcode/Reader/BitcodeReader.cpp
>>>>>   M lib/CodeGen/SelectionDAG/FastISel.cpp
>>>>>   M lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
>>>>>   M lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>>>>>   M lib/ExecutionEngine/ExecutionEngine.cpp
>>>>>   M lib/Support/APFloat.cpp
>>>>>   M lib/Support/APInt.cpp
>>>>>   M lib/VMCore/ConstantFold.cpp
>>>>>   M lib/VMCore/Core.cpp
>>>>>   M unittests/ADT/APIntTest.cpp
>>>
>>> @@ -342,7 +353,8 @@ public:
>>>
>>>     if (isSingleWord())
>>>       return isUIntN(N, VAL);
>>> -    return APInt(N, getNumWords(), pVal).zext(getBitWidth()) == (*this);
>>> +    return APInt(N, ArrayRef<uint64_t>(pVal,
>>> getNumWords())).zext(getBitWidth())
>>> +      == (*this);
>>>   }
>>>
>>>   /// @brief Check if this APInt has an N-bits signed integer value.
>>>
>>> makeArrayRef just landed; use it. :)  Same in a couple other places.
>>
>> Silly codebase, changing out from under me. ;-) Will do.
>>
>>> Otherwise, looks fine.
>>>
>>> As a followup, could you eliminate both the "APInt(unsigned numBits,
>>> unsigned numWords, const uint64_t bigVal[])" and the "APInt(unsigned
>>> numBits, uint64_t val, bool isSigned = false)" constructors?  (It
>>> should just require ArrayRef-izing and adding an
>>> "APInt::getSigned(unsigned numBits, uint64_t val)" helper.)
>>
>> How do you want to handle dependent codebases? I was thinking to leave
>> APInt(...bigVal[]) in for a version to let people switch, but I guess
>> LLVM does tend to remove things more aggressively than that. I'm only
>> willing to commit to fixing clang myself if you want me to both fix
>> things and remove the deprecated constructors.
>
> By which I mean that I'm not volunteering to fix anything besides
> clang, but I am happy to fix clang.

K; I'll fix llvm-gcc; and it should be easy enough to fix dragonegg.

> Also, do you want APInt::getSigned() to take a uint64_t or an int64_t?
> IIRC, taking uint64_t introduces some warnings when we pass literal
> negative numbers, and we made ConstantInt::getSigned() take an int64_t
> for that reason. Hm, without the bool-taking version of APInt(),
> ConstantInt::get() will need an if, or we'll have to change it to not
> accept signed numbers either.

I guess int64_t, since it is expecting a signed value. :)  Making
ConstantInt::get use an if doesn't seem like a big deal.

-Eli