[llvm] r300477 - [APInt] Remove self move check from move assignment operator
Reid Kleckner via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 24 14:15:01 PDT 2018
PTAL: https://reviews.llvm.org/D46028
On Tue, Apr 24, 2018 at 2:01 PM Craig Topper <craig.topper at gmail.com> wrote:
> Put the self move check at the top of the function. Not buried in the
> !isSingleWord case. You need to protect the memcpy too.
>
> ~Craig
>
> On Tue, Apr 24, 2018 at 1:59 PM, Reid Kleckner <rnk at google.com> wrote:
>
>> I misunderstood, I thought this was done in April 17 2018, not 2017. It's
>> not so easy.
>>
>> But yes, I was going to revert the removal of the self-assignment check.
>>
>> On Tue, Apr 24, 2018 at 1:59 PM Craig Topper <craig.topper at gmail.com>
>> wrote:
>>
>>> Revert the APInt change?
>>>
>>> ~Craig
>>>
>>> On Tue, Apr 24, 2018 at 1:57 PM, Reid Kleckner <rnk at google.com> wrote:
>>>
>>>> I'll go ahead and revert for now to fix the bot. Its unclear to me what
>>>> we should do long term.
>>>>
>>>> On Sat, Apr 21, 2018 at 6:11 PM Chandler Carruth <chandlerc at gmail.com>
>>>> wrote:
>>>>
>>>>> Sorry to do some commit necromancy...
>>>>>
>>>>> But our Windows expensive check bots fail because of this:
>>>>>
>>>>> http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/9193/steps/test-check-all/logs/stdio
>>>>>
>>>>> It seems that while std::stable_sort no longer does self moves,
>>>>> std::shuffle still does.
>>>>>
>>>>> Thoughts?
>>>>> -Chandler
>>>>>
>>>>> On Mon, Apr 17, 2017 at 11:57 AM Craig Topper via llvm-commits <
>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>
>>>>>> Author: ctopper
>>>>>> Date: Mon Apr 17 13:44:27 2017
>>>>>> New Revision: 300477
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=300477&view=rev
>>>>>> Log:
>>>>>> [APInt] Remove self move check from move assignment operator
>>>>>>
>>>>>> This was added to work around a bug in MSVC 2013's implementation of
>>>>>> stable_sort. That bug has been fixed as of MSVC 2015 so we shouldn't need
>>>>>> this anymore.
>>>>>>
>>>>>> Technically the current implementation has undefined behavior because
>>>>>> we only protect the deleting of the pVal array with the self move check.
>>>>>> There is still a memcpy of that.VAL to VAL that isn't protected. In the
>>>>>> case of self move those are the same local and memcpy is undefined for src
>>>>>> and dst overlapping.
>>>>>>
>>>>>> This reduces the size of the opt binary on my local x86-64 build by
>>>>>> about 4k.
>>>>>>
>>>>>> Differential Revision: https://reviews.llvm.org/D32116
>>>>>>
>>>>>>
>>>>>>
>>>>>> Modified:
>>>>>> llvm/trunk/include/llvm/ADT/APInt.h
>>>>>> llvm/trunk/unittests/ADT/APIntTest.cpp
>>>>>>
>>>>>> Modified: llvm/trunk/include/llvm/ADT/APInt.h
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/APInt.h?rev=300477&r1=300476&r2=300477&view=diff
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- llvm/trunk/include/llvm/ADT/APInt.h (original)
>>>>>> +++ llvm/trunk/include/llvm/ADT/APInt.h Mon Apr 17 13:44:27 2017
>>>>>> @@ -693,24 +693,16 @@ public:
>>>>>>
>>>>>> /// @brief Move assignment operator.
>>>>>> APInt &operator=(APInt &&that) {
>>>>>> - if (!isSingleWord()) {
>>>>>> - // The MSVC STL shipped in 2013 requires that self move
>>>>>> assignment be a
>>>>>> - // no-op. Otherwise algorithms like stable_sort will produce
>>>>>> answers
>>>>>> - // where half of the output is left in a moved-from state.
>>>>>> - if (this == &that)
>>>>>> - return *this;
>>>>>> + assert(this != &that && "Self-move not supported");
>>>>>> + if (!isSingleWord())
>>>>>> delete[] pVal;
>>>>>> - }
>>>>>>
>>>>>> // Use memcpy so that type based alias analysis sees both VAL
>>>>>> and pVal
>>>>>> // as modified.
>>>>>> memcpy(&VAL, &that.VAL, sizeof(uint64_t));
>>>>>>
>>>>>> - // If 'this == &that', avoid zeroing our own bitwidth by storing
>>>>>> to 'that'
>>>>>> - // first.
>>>>>> - unsigned ThatBitWidth = that.BitWidth;
>>>>>> + BitWidth = that.BitWidth;
>>>>>> that.BitWidth = 0;
>>>>>> - BitWidth = ThatBitWidth;
>>>>>>
>>>>>> return *this;
>>>>>> }
>>>>>>
>>>>>> Modified: llvm/trunk/unittests/ADT/APIntTest.cpp
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/APIntTest.cpp?rev=300477&r1=300476&r2=300477&view=diff
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- llvm/trunk/unittests/ADT/APIntTest.cpp (original)
>>>>>> +++ llvm/trunk/unittests/ADT/APIntTest.cpp Mon Apr 17 13:44:27 2017
>>>>>> @@ -1606,36 +1606,6 @@ TEST(APIntTest, isShiftedMask) {
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> -#if defined(__clang__)
>>>>>> -// Disable the pragma warning from versions of Clang without
>>>>>> -Wself-move
>>>>>> -#pragma clang diagnostic push
>>>>>> -#pragma clang diagnostic ignored "-Wunknown-pragmas"
>>>>>> -// Disable the warning that triggers on exactly what is being tested.
>>>>>> -#pragma clang diagnostic push
>>>>>> -#pragma clang diagnostic ignored "-Wself-move"
>>>>>> -#endif
>>>>>> -TEST(APIntTest, SelfMoveAssignment) {
>>>>>> - APInt X(32, 0xdeadbeef);
>>>>>> - X = std::move(X);
>>>>>> - EXPECT_EQ(32u, X.getBitWidth());
>>>>>> - EXPECT_EQ(0xdeadbeefULL, X.getLimitedValue());
>>>>>> -
>>>>>> - uint64_t Bits[] = {0xdeadbeefdeadbeefULL, 0xdeadbeefdeadbeefULL};
>>>>>> - APInt Y(128, Bits);
>>>>>> - Y = std::move(Y);
>>>>>> - EXPECT_EQ(128u, Y.getBitWidth());
>>>>>> - EXPECT_EQ(~0ULL, Y.getLimitedValue());
>>>>>> - const uint64_t *Raw = Y.getRawData();
>>>>>> - EXPECT_EQ(2u, Y.getNumWords());
>>>>>> - EXPECT_EQ(0xdeadbeefdeadbeefULL, Raw[0]);
>>>>>> - EXPECT_EQ(0xdeadbeefdeadbeefULL, Raw[1]);
>>>>>> -}
>>>>>> -#if defined(__clang__)
>>>>>> -#pragma clang diagnostic pop
>>>>>> -#pragma clang diagnostic pop
>>>>>> -#endif
>>>>>> -}
>>>>>> -
>>>>>> TEST(APIntTest, reverseBits) {
>>>>>> EXPECT_EQ(1, APInt(1, 1).reverseBits());
>>>>>> EXPECT_EQ(0, APInt(1, 0).reverseBits());
>>>>>> @@ -2025,3 +1995,5 @@ TEST(APIntTest, GCD) {
>>>>>> APInt C = GreatestCommonDivisor(A, B);
>>>>>> EXPECT_EQ(C, HugePrime);
>>>>>> }
>>>>>> +
>>>>>> +} // end anonymous namespace
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> llvm-commits mailing list
>>>>>> llvm-commits at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>
>>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180424/e247b9fe/attachment.html>
More information about the llvm-commits
mailing list