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

Tue Apr 24 13:59:50 PDT 2018

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