[llvm] r300477 - [APInt] Remove self move check from move assignment operator

[Craig Topper via llvm-commits]

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/bf459732/attachment.html>