This is an archive of the discontinued LLVM Phabricator instance.

[MC] Disable copying and moving of MCInstrDescs the C++20 way
Needs RevisionPublic

Authored by foad on Jan 24 2023, 2:57 AM.

Diff Detail

Event Timeline

foad created this revision.Jan 24 2023, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2023, 2:57 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
foad requested review of this revision.Jan 24 2023, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2023, 2:57 AM
barannikov88 added inline comments.
llvm/include/llvm/MC/MCInstrDesc.h
211

Should also be made final.

Ah, I keep forgetting that the unknown attributes are fine. My experiments with MSVC suggest that it will increase the struct size: https://gcc.godbolt.org/z/offYrhn4K.
libc++ uses msvc::no_unique_address, which seems to work, but is MSVC-specific.

We should hide the [[no_unique_address]] behind a macro, we do the same for LLVM_REQUIRE_CONSTANT_INITIALIZATION and LLVM_FALLTHROUGH.
This should allow to use MSVC-specific version and avoid warnings in compilers that don't support no_unique_address (I'm not entirely certain, but suspect LLVM might still require building with those).

llvm/include/llvm/MC/MCInstrDesc.h
193

We should not use an anonymous namespace in headers.
This causes MCInstrDesc to have a unique declaration in each .cpp file it's used, leading to ODR violations.

foad updated this revision to Diff 491693.Jan 24 2023, 3:19 AM

Add final.

foad updated this revision to Diff 491694.Jan 24 2023, 3:21 AM

Remove anonymous namespace.

foad marked 2 inline comments as done.Jan 24 2023, 3:22 AM
foad added inline comments.Jan 24 2023, 6:16 AM
llvm/include/llvm/MC/MCInstrDesc.h
226

Ugh, I had not realized that in C++17 this attribute is unrecognized and does nothing, so the NCNM member increases the size of this struct from 48 bytes to 56 bytes (on 64-bit hosts). Maybe I should give up and leave the FIXME in place until we have adopted C++20.

Alternatively, how bad would it be if aggregate initialization were removed and these objects were initialized with ctor calls? (bad for an -O0 build, no doubt... (how bad?) calling all those ctors to build the tables, but does clang optimize all the ctor calls away to the equivalent of aggregate init when using optimizations?)

Alternatively, how bad would it be if aggregate initialization were removed and these objects were initialized with ctor calls? (bad for an -O0 build, no doubt... (how bad?) calling all those ctors to build the tables, but does clang optimize all the ctor calls away to the equivalent of aggregate init when using optimizations?)

The alternative suggestion would be to add some library support, roughly this:

#include <cstdint>

struct NonCopyableNonMovable {
  NonCopyableNonMovable() = default;
  NonCopyableNonMovable(const NonCopyableNonMovable&) = delete;
  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
  NonCopyableNonMovable &operator=(const NonCopyableNonMovable &) = delete;
  NonCopyableNonMovable &operator=(NonCopyableNonMovable &&) = delete;
};

#if defined(__has_cpp_attribute) && __has_cpp_attribute(no_unique_address)
#define LLVM_NO_UNIQUE_ADDRESS [[no_unique_address]]
#elif defined(__has_cpp_attribute) && __has_cpp_attribute(msvc::no_unique_address)
#define LLVM_NO_UNIQUE_ADDRESS [[msvc::no_unique_address]]
#else
#error "WTF"
#define LLVM_NO_UNIQUE_ADDRESS
#endif

#if __cplusplus >= 202002L
// Put as the last member in the aggregate struct.
#define LLVM_DISABLE_COPY_AND_MOVE(X)\
    LLVM_NO_UNIQUE_ADDRESS NonCopyableNonMovable DisableCopy##X;
#else
#define LLVM_DISABLE_COPY_AND_MOVE(X)\
    X(X const&) = delete;\
    X(X&&) = delete;\
    X& operator=(X const&) = delete;\
    X& operator=(X&&) = delete;
#endif

And use it like this:

struct Y {
    uint64_t a;
    uint64_t b;

    LLVM_DISABLE_COPY_AND_MOVE(Y);
};
auto y = Y{1, 2};
static_assert(sizeof(Y) == 16);

Seem to work in both C++17 and C++20, except in the latest MSVC (looks like a bug, but I'm not entirely certain):
https://gcc.godbolt.org/z/8nsooKz8c
I suspect this could be worked around, but I did not dig any further.

The question is: how useful is it and does it carry its weight?

Alternatively, how bad would it be if aggregate initialization were removed and these objects were initialized with ctor calls? (bad for an -O0 build, no doubt... (how bad?) calling all those ctors to build the tables, but does clang optimize all the ctor calls away to the equivalent of aggregate init when using optimizations?)

The alternative suggestion would be to add some library support, roughly this:

#include <cstdint>

struct NonCopyableNonMovable {
  NonCopyableNonMovable() = default;
  NonCopyableNonMovable(const NonCopyableNonMovable&) = delete;
  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
  NonCopyableNonMovable &operator=(const NonCopyableNonMovable &) = delete;
  NonCopyableNonMovable &operator=(NonCopyableNonMovable &&) = delete;
};

#if defined(__has_cpp_attribute) && __has_cpp_attribute(no_unique_address)
#define LLVM_NO_UNIQUE_ADDRESS [[no_unique_address]]
#elif defined(__has_cpp_attribute) && __has_cpp_attribute(msvc::no_unique_address)
#define LLVM_NO_UNIQUE_ADDRESS [[msvc::no_unique_address]]
#else
#error "WTF"
#define LLVM_NO_UNIQUE_ADDRESS
#endif

#if __cplusplus >= 202002L
// Put as the last member in the aggregate struct.
#define LLVM_DISABLE_COPY_AND_MOVE(X)\
    LLVM_NO_UNIQUE_ADDRESS NonCopyableNonMovable DisableCopy##X;
#else
#define LLVM_DISABLE_COPY_AND_MOVE(X)\
    X(X const&) = delete;\
    X(X&&) = delete;\
    X& operator=(X const&) = delete;\
    X& operator=(X&&) = delete;
#endif

And use it like this:

struct Y {
    uint64_t a;
    uint64_t b;

    LLVM_DISABLE_COPY_AND_MOVE(Y);
};
auto y = Y{1, 2};
static_assert(sizeof(Y) == 16);

Seem to work in both C++17 and C++20, except in the latest MSVC (looks like a bug, but I'm not entirely certain):
https://gcc.godbolt.org/z/8nsooKz8c
I suspect this could be worked around, but I did not dig any further.

The question is: how useful is it and does it carry its weight?

Sounds OK to me, but yeah, might be overkill.

@foad any sense of how important this is? whether it's worth the above compelxity?

barannikov88 added a comment.EditedJan 25 2023, 5:49 PM

What if not introduce a dummy class but instead extract a part of MCInstrDesc that is not directly used by clients? E.g.

struct Hack {
  unsigned short Opc;
  constexpr Hack(unsigned short Opc) : Opc(Opc) {}
  Hack (const Hack&) = delete;
  Hack &operator=(const Hack&) = delete;
};

struct MCInstrDesc {
  Hack Opcode;
  unsigned short NumOperands;
};

MCInstrDesc global{{1}, 2};

https://gcc.godbolt.org/z/nchKhM1Wd

No attribute is required.

ADD
It would be kind of natural to move the non-copyable newly introduced operands offsets there. They aren't user accessible either.

I don’t understand this C++ change. How is this not just worse? Why did they do this?

foad added a comment.Jan 26 2023, 5:13 AM

I don’t understand this C++ change. How is this not just worse? Why did they do this?

The paper explains in detail: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf

foad added a comment.Jan 27 2023, 5:45 AM

Sounds OK to me, but yeah, might be overkill.

@foad any sense of how important this is? whether it's worth the above compelxity?

I don't think it's critical to have this checking, it's just nice to have. Personally I would be fine with a C++20-only solution (under an appropriate #if), and live without the checking until we adopt C++20. (And even before that we would get some checking from the C++20 builders.)

Sounds OK to me, but yeah, might be overkill.

@foad any sense of how important this is? whether it's worth the above compelxity?

I don't think it's critical to have this checking, it's just nice to have. Personally I would be fine with a C++20-only solution (under an appropriate #if), and live without the checking until we adopt C++20. (And even before that we would get some checking from the C++20 builders.)

Given the relatively small adoption of C++20 just yet, I'd hesitate to put more burden on those maintaining/working towards C++20 support to cleanup failures that aren't the minimum necessary for C++20 support - if it's not validated by a more mainstream configuration, I think it's best not to test it only in C++20 mode.

arsenm added inline comments.Aug 17 2023, 3:15 PM
llvm/include/llvm/MC/MCInstrDesc.h
195

drop the c++17 part if it's not actually true?

arsenm requested changes to this revision.Aug 23 2023, 4:42 PM

Moving out of review queue

This revision now requires changes to proceed.Aug 23 2023, 4:42 PM