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

[foad]

Inspired by the paper that fixed C++20, P1008R1: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf

[barannikov88]

llvm/include/llvm/MC/MCInstrDesc.h

209

Should also be made final.

[ilya-biryukov]

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]

Add final.

[foad]

Remove anonymous namespace.

[foad]

llvm/include/llvm/MC/MCInstrDesc.h

224

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.

[dblaikie]

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?)

[ilya-biryukov]

In D142446#4078204, @dblaikie wrote:

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?

[dblaikie]

In D142446#4079439, @ilya-biryukov wrote:

In D142446#4078204, @dblaikie wrote:

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]

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.

[arsenm]

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

[foad]

In D142446#4082332, @arsenm wrote:

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]

In D142446#4081425, @dblaikie wrote:

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.)

[dblaikie]

In D142446#4085389, @foad wrote:

In D142446#4081425, @dblaikie wrote:

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]

llvm/include/llvm/MC/MCInstrDesc.h

195

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

[arsenm]

Moving out of review queue

llvm/include/llvm/MC/MCInstrDesc.h

enum Flag {
   Convergent,
   Add,
   Trap,
   VariadicOpsAreDefs,
   Authenticated,
 };
 } // namespace MCID
 
+// An empty struct that can be used as a trailing member to prevent another
+// struct from being copied or moved while still allowing aggregate
+// initialization in both C++17 and C++20.
+struct NonCopyableNonMovable {
+  NonCopyableNonMovable() = default;
+  NonCopyableNonMovable(const NonCopyableNonMovable&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable &operator=(const NonCopyableNonMovable &) = delete;
+  NonCopyableNonMovable &operator=(NonCopyableNonMovable &&) = delete;
+};
+
 /// Describe properties that are true of each instruction in the target
 /// description file.  This captures information about side effects, register
 /// use and many other things.  There is one instance of this struct for each
 /// target instruction class, and the MachineInstr class points to this struct
 /// directly to describe itself.
-class MCInstrDesc {
+class MCInstrDesc final {
 public:
-  // FIXME: Disable copies and moves.
-  // Do not allow MCInstrDescs to be copied or moved. They should only exist in
-  // the <Target>Insts table because they rely on knowing their own address to
-  // find other information elsewhere in the same table.
-
   unsigned short Opcode;         // The opcode number
   unsigned short NumOperands;    // Num of args (may be more if variable_ops)
   unsigned char NumDefs;         // Num of args that are definitions
   unsigned char Size;            // Number of bytes in encoding.
   unsigned short SchedClass;     // enum identifying instr sched class
   uint64_t Flags;                // Flags identifying machine instr class
   uint64_t TSFlags;              // Target Specific Flag values
   const MCPhysReg *ImplicitUses; // Registers implicitly read by this instr
   const MCPhysReg *ImplicitDefs; // Registers implicitly defined by this instr
   const MCOperandInfo *OpInfo;   // 'NumOperands' entries about operands
+  // Do not allow MCInstrDescs to be copied or moved. They should only exist in
+  // the <Target>Insts table because they rely on knowing their own address to
+  // find other information elsewhere in the same table.
+  [[no_unique_address]] NonCopyableNonMovable NCNM{};
 
   /// Returns the value of the specified operand constraint if
   /// it is present. Returns -1 if it is not present.
   int getOperandConstraint(unsigned OpNum,
                            MCOI::OperandConstraint Constraint) const {
     if (OpNum < NumOperands &&
         (operands()[OpNum].Constraints & (1 << Constraint))) {
       unsigned ValuePos = 4 + Constraint * 4;