[llvm] r300477 - [APInt] Remove self move check from move assignment operator
Craig Topper via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 24 14:01:50 PDT 2018
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/1081e4b4/attachment.html>
More information about the llvm-commits
mailing list