[MC] Do not copy MCInstrDescs. NFC.

[foad]

Summary

Avoid copying MCInstrDesc instances because a future patch will change
them to find their implicit operands and operand info array based on
their own "this" pointer, so it will only work for MCInstrDescs in the
TargetInsts table, not for a copy of an MCInstrDesc at a different
address.

[foad]

llvm/include/llvm/MC/MCInstrDesc.h

200

@dblaikie or any other C++ experts - does this seem reasonable? I was not sure if I should be following some kind of rule-of-3/5/7/11/13/... here. All I want to do is prevent users from accidentally writing MCInstrDesc MCID = ... instead of const MCInstrDesc &MCID = ....

[barannikov88]

llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCCodeEmitter.cpp

64

Doesn't this trigger compilation error (missing const)?

[foad]

llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCCodeEmitter.cpp

64

Yes! Apparently you have to enable this target via LLVM_EXPERIMENTAL_TARGETS_TO_BUILD and I did not test that. Thanks for spotting it.

[foad]

Fix SPIRV target.

[dblaikie]

llvm/include/llvm/MC/MCInstrDesc.h

200

Generally I'd suggest that APIs shouldn't make awkward ergonomic choices to reduce the chance of accidents - there's lots of things that can be expensive to copy in C++ that still have the usual copy ctor/assignment operator.

So if the thing is copyable - forcing users to do that copy in some uncommon way because the usual way is too error prone is, imho, not the right way to go - though I realize trying to teach people that this thing is expensive to copy isn't easy either. Hopefully updating a bunch of places that were copying will help set the right expectations.

If it's really non-copyable in any way, then, yes, the rule of 5 would apply. Implement the copy ctor, copy assignment, move ctor, move assignment, and destructor.

Sounds like deleting all the copy/move operations is what you want? Leave the dtor defaulted, maybe default the default (no-args) ctor? since that seems to be used in a test? (honestly, how are these objects constructed otherwise? I guess with aggregate init?)

[foad]

llvm/include/llvm/MC/MCInstrDesc.h

200

Yes they are created with aggregate init, in a big static table generated by tablegen.

They are not expensive to copy. They "can't" be copied in that some methods would no longer work correctly, because they rely on being able to access other data in the same table at a known offset from "this".

[dblaikie]

llvm/include/llvm/MC/MCInstrDesc.h

200

Ah, OK - yeah, totally disabling copy/move ops seems fine, then. So probably deleting all copy/move ctors, defaulting the dtor (I think that might still be implicit, despite the deleted copy/move ops, so you can leave that if it works without explicitly defaulting it).
I'm surprised the aggregate init works given the deleted copy ctor here, but if it works, it works I guess *shrug*

[foad]

"Rule of 4" (rule of 5 without the desctructor).

[foad]

llvm/include/llvm/MC/MCInstrDesc.h

200

Done, thanks!

[sammccall]

TL;DR: this doesn't work in C++20, maybe revert?

I'm surprised the aggregate init works given the deleted copy ctor here, but if it works, it works I guess *shrug*

The aggregate init works up to C++17, but not in C++20.
(The rules change from "no user-provided, inherited, or explicit constructors" to "no user-declared or inherited constructors").

I think the current status of building LLVM in C++20 mode is "theoretically supported, but no stable buildbot yet". It's being worked on.

Since C++20 compatibility is something we'll need to do in the medium-term, and incompatible constructs make building a good bot harder in the short-term, I'd reluctantly suggest reverting this.

(Eliminating the aggregate init or finding an alternative technique to break copy/move also seem fine, starting with a revert would still be nice if it's not trivial)

[foad]

In D142214#4076194, @sammccall wrote:

TL;DR: this doesn't work in C++20, maybe revert?

I'm surprised the aggregate init works given the deleted copy ctor here, but if it works, it works I guess *shrug*

The aggregate init works up to C++17, but not in C++20.
(The rules change from "no user-provided, inherited, or explicit constructors" to "no user-declared or inherited constructors").

I think the current status of building LLVM in C++20 mode is "theoretically supported, but no stable buildbot yet". It's being worked on.

Since C++20 compatibility is something we'll need to do in the medium-term, and incompatible constructs make building a good bot harder in the short-term, I'd reluctantly suggest reverting this.

(Eliminating the aggregate init or finding an alternative technique to break copy/move also seem fine, starting with a revert would still be nice if it's not trivial)

Interesting. Is there any easy way I can test building in C++20 mode on my own machine?

[sammccall]

In D142214#4076245, @foad wrote:

In D142214#4076194, @sammccall wrote:

TL;DR: this doesn't work in C++20, maybe revert?

I'm surprised the aggregate init works given the deleted copy ctor here, but if it works, it works I guess *shrug*

The aggregate init works up to C++17, but not in C++20.
(The rules change from "no user-provided, inherited, or explicit constructors" to "no user-declared or inherited constructors").

I think the current status of building LLVM in C++20 mode is "theoretically supported, but no stable buildbot yet". It's being worked on.

Since C++20 compatibility is something we'll need to do in the medium-term, and incompatible constructs make building a good bot harder in the short-term, I'd reluctantly suggest reverting this.

(Eliminating the aggregate init or finding an alternative technique to break copy/move also seem fine, starting with a revert would still be nice if it's not trivial)

Interesting. Is there any easy way I can test building in C++20 mode on my own machine?

Looks like you should set -DCMAKE_CXX_STANDARD=20 (this lets me reproduce this class of error locally, with clang++-12 as host compiler).

[ilya-biryukov]

In D142214#4076245, @foad wrote:

Interesting. Is there any easy way I can test building in C++20 mode on my own machine?

cmake -DCMAKE_CXX_STANDARD=20 should give the C++20 build. However, make sure to have a ~new compiler. Clang 15 or GCC 12 should do the trick.

I have sent https://github.com/llvm/llvm-project/commit/f3d64644d8d75f9317421fe5b9b0f2d030a1a63f to unbreak our integrate, but actually want to help re-enable the enforcement that the type is not copied today.

[ilya-biryukov]

To describe the problem, the following code breaks in C++20:

struct WantAggregate {
  WantAggregate(const WantAggregate&) = delete;
  int a;
  int b;
};

WantAggregate a = {1,2}; // does not compile in C++20.

One way to workaround this would be to force an implicitly-deleted constructor:

struct NoCopy {
  NoCopy() = default;
  NoCopy(const &) = delete;
};
struct WantAggregate {
  int a;
  int b;
  NoCopy delete_copy = {};
};

WantAggregate a = {1,2}; // works!

However, this approach increases the size of MCInstrDesc (the extra member needs to have an address). One way to workaround it is

[[no_unique_address]] NoCopy delete_copy = {}; // no increase in size, implicitly deleted copy!

Unfortunately, this attribute was added only in C++20. I am trying to figure out a way to make this work in both C++17 and C++20 without the size increase, but still struggling to find a way without a preprocessor.

(I'm assuming the size increase is undesirable, lmk if it's actually not a big deal. Using a member is probably the easiest workaround.)

[ilya-biryukov]

For the record, the corresponding standard change is P1008R1.

[foad]

C++20 build should be fixed properly by D142446

[rafauler]

This breaks the BOLT build.

https://lab.llvm.org/buildbot/#/builders/244/builds/5433

[rafauler]

In D142214#4077785, @rafauler wrote:

This breaks the BOLT build.

https://lab.llvm.org/buildbot/#/builders/244/builds/5433

Nevermind, this is already fixed. Thanks.

llvm/include/llvm/MC/MCInstrDesc.h

 
 /// 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 {
 public:
+  // 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.
+  MCInstrDesc(const MCInstrDesc &) = delete;
+  MCInstrDesc(MCInstrDesc &&) = delete;
+  MCInstrDesc &operator=(const MCInstrDesc &) = delete;
+  MCInstrDesc &operator=(MCInstrDesc &&) = delete;
+
   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

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp

if (Opcode == TargetOpcode::REG_SEQUENCE) {
       unsigned DstRCIdx = cast<ConstantSDNode>(Node->getOperand(0))->getZExtValue();
       const TargetRegisterClass *RC = TRI->getRegClass(DstRCIdx);
       RegClass = RC->getID();
       Cost = RegSequenceCost;
       return;
     }
 
     unsigned Idx = RegDefPos.GetIdx();
-    const MCInstrDesc Desc = TII->get(Opcode);
+    const MCInstrDesc &Desc = TII->get(Opcode);
     const TargetRegisterClass *RC = TII->getRegClass(Desc, Idx, TRI, MF);
     assert(RC && "Not a valid register class");
     RegClass = RC->getID();
     // FIXME: Cost arbitrarily set to 1 because there doesn't seem to be a
     // better way to determine it.
     Cost = 1;
   } else {
     RegClass = TLI->getRepRegClassFor(VT)->getID();

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

for (SDNode::use_iterator U = N->use_begin(), E = SDNode::use_end();
     if (!RC || SIRI->isSGPRClass(RC))
       return false;
 
     if (RC != &AMDGPU::VS_32RegClass) {
       AllUsesAcceptSReg = false;
       SDNode * User = *U;
       if (User->isMachineOpcode()) {
         unsigned Opc = User->getMachineOpcode();
-        MCInstrDesc Desc = SII->get(Opc);
+        const MCInstrDesc &Desc = SII->get(Opc);
         if (Desc.isCommutable()) {
           unsigned OpIdx = Desc.getNumDefs() + U.getOperandNo();
           unsigned CommuteIdx1 = TargetInstrInfo::CommuteAnyOperandIndex;
           if (SII->findCommutedOpIndices(Desc, OpIdx, CommuteIdx1)) {
             unsigned CommutedOpNo = CommuteIdx1 - Desc.getNumDefs();
             const TargetRegisterClass *CommutedRC = getOperandRegClass(*U, CommutedOpNo);
             if (CommutedRC == &AMDGPU::VS_32RegClass)
               AllUsesAcceptSReg = true;

llvm/lib/Target/AMDGPU/SIInstrInfo.h

public:
   LLVM_READONLY
   inline int commuteOpcode(const MachineInstr &MI) const {
     return commuteOpcode(MI.getOpcode());
   }
 
   bool findCommutedOpIndices(const MachineInstr &MI, unsigned &SrcOpIdx0,
                              unsigned &SrcOpIdx1) const override;
 
-  bool findCommutedOpIndices(MCInstrDesc Desc, unsigned &SrcOpIdx0,
+  bool findCommutedOpIndices(const MCInstrDesc &Desc, unsigned &SrcOpIdx0,
                              unsigned &SrcOpIdx1) const;
 
   bool isBranchOffsetInRange(unsigned BranchOpc,
                              int64_t BrOffset) const override;
 
   MachineBasicBlock *getBranchDestBlock(const MachineInstr &MI) const override;
 
   /// Return whether the block terminate with divergent branch.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

 // between the true commutable operands, and the base
 // TargetInstrInfo::commuteInstruction uses it.
 bool SIInstrInfo::findCommutedOpIndices(const MachineInstr &MI,
                                         unsigned &SrcOpIdx0,
                                         unsigned &SrcOpIdx1) const {
   return findCommutedOpIndices(MI.getDesc(), SrcOpIdx0, SrcOpIdx1);
 }
 
-bool SIInstrInfo::findCommutedOpIndices(MCInstrDesc Desc, unsigned &SrcOpIdx0,
+bool SIInstrInfo::findCommutedOpIndices(const MCInstrDesc &Desc,
+                                        unsigned &SrcOpIdx0,
                                         unsigned &SrcOpIdx1) const {
   if (!Desc.isCommutable())
     return false;
 
   unsigned Opc = Desc.getOpcode();
   int Src0Idx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::src0);
   if (Src0Idx == -1)
     return false;

llvm/lib/Target/Lanai/LanaiDelaySlotFiller.cpp

bool Filler::delayHasHazard(MachineBasicBlock::instr_iterator MI, bool &SawLoad,
   return false;
 }
 
 // Insert Defs and Uses of MI into the sets RegDefs and RegUses.
 void Filler::insertDefsUses(MachineBasicBlock::instr_iterator MI,
                             SmallSet<unsigned, 32> &RegDefs,
                             SmallSet<unsigned, 32> &RegUses) {
   // If MI is a call or return, just examine the explicit non-variadic operands.
-  MCInstrDesc MCID = MI->getDesc();
+  const MCInstrDesc &MCID = MI->getDesc();
   unsigned E = MI->isCall() || MI->isReturn() ? MCID.getNumOperands()
                                               : MI->getNumOperands();
   for (unsigned I = 0; I != E; ++I) {
     const MachineOperand &MO = MI->getOperand(I);
     unsigned Reg;
 
     if (!MO.isReg() || !(Reg = MO.getReg()))
       continue;

llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVInstPrinter.cpp

void SPIRVInstPrinter::printInst(const MCInst *MI, uint64_t Address,
   if (OpCode == SPIRV::OpDecorate) {
     printOpDecorate(MI, OS);
   } else if (OpCode == SPIRV::OpExtInstImport) {
     recordOpExtInstImport(MI);
   } else if (OpCode == SPIRV::OpExtInst) {
     printOpExtInst(MI, OS);
   } else {
     // Print any extra operands for variadic instructions.
-    MCInstrDesc MCDesc = MII.get(OpCode);
+    const MCInstrDesc &MCDesc = MII.get(OpCode);
     if (MCDesc.isVariadic()) {
       const unsigned NumFixedOps = MCDesc.getNumOperands();
       const unsigned LastFixedIndex = NumFixedOps - 1;
       const int FirstVariableIndex = NumFixedOps;
       if (NumFixedOps > 0 && MCDesc.operands()[LastFixedIndex].OperandType ==
                                  MCOI::OPERAND_UNKNOWN) {
         // For instructions where a custom type (not reg or immediate) comes as
         // the last operand before the variable_ops. This is usually a StringImm
void SPIRVInstPrinter::printInst(const MCInst *MI, uint64_t Address,
   }
 
   printAnnotation(OS, Annot);
 }
 
 void SPIRVInstPrinter::printOpExtInst(const MCInst *MI, raw_ostream &O) {
   // The fixed operands have already been printed, so just need to decide what
   // type of ExtInst operands to print based on the instruction set and number.
-  MCInstrDesc MCDesc = MII.get(MI->getOpcode());
+  const MCInstrDesc &MCDesc = MII.get(MI->getOpcode());
   unsigned NumFixedOps = MCDesc.getNumOperands();
   const auto NumOps = MI->getNumOperands();
   if (NumOps == NumFixedOps)
     return;
 
   O << ' ';
 
   // TODO: implement special printing for OpenCLExtInst::vstor*.
   printRemainingVariableOps(MI, NumFixedOps, O, true);
 }
 
 void SPIRVInstPrinter::printOpDecorate(const MCInst *MI, raw_ostream &O) {
   // The fixed operands have already been printed, so just need to decide what
   // type of decoration operands to print based on the Decoration type.
-  MCInstrDesc MCDesc = MII.get(MI->getOpcode());
+  const MCInstrDesc &MCDesc = MII.get(MI->getOpcode());
   unsigned NumFixedOps = MCDesc.getNumOperands();
 
   if (NumFixedOps != MI->getNumOperands()) {
     auto DecOp = MI->getOperand(NumFixedOps - 1);
     auto Dec = static_cast<Decoration::Decoration>(DecOp.getImm());
 
     O << ' ';
 

llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVMCCodeEmitter.cpp

 }
 
 using EndianWriter = support::endian::Writer;
 
 // Check if the instruction has a type argument for operand 1, and defines an ID
 // output register in operand 0. If so, we need to swap operands 0 and 1 so the
 // type comes first in the output, despide coming second in the MCInst.
 static bool hasType(const MCInst &MI, const MCInstrInfo &MII) {
-  MCInstrDesc MCDesc = MII.get(MI.getOpcode());
+  const MCInstrDesc &MCDesc = MII.get(MI.getOpcode());
   // If we define an output, and have at least one other argument.
   if (MCDesc.getNumDefs() == 1 && MCDesc.getNumOperands() >= 2) {
     // Check if we define an ID, and take a type as operand 1.
     auto &DefOpInfo = MCDesc.operands()[0];
     auto &FirstArgOpInfo = MCDesc.operands()[1];
     return (DefOpInfo.RegClass == SPIRV::IDRegClassID ||
             DefOpInfo.RegClass == SPIRV::ANYIDRegClassID) &&
            FirstArgOpInfo.RegClass == SPIRV::TYPERegClassID;

llvm/unittests/CodeGen/LexicalScopesTest.cpp

public:
   // Test environment blocks -- these form a diamond control flow pattern,
   // MBB1 being the entry block, blocks two and three being the branches, and
   // block four joining the branches and being an exit block.
   MachineBasicBlock *MBB1, *MBB2, *MBB3, *MBB4;
 
   // Some meaningless instructions -- the first is fully meaningless,
   // while the second is supposed to impersonate DBG_VALUEs through its
   // opcode.
-  MCInstrDesc BeanInst;
-  MCInstrDesc DbgValueInst;
+  MCInstrDesc BeanInst{};
+  MCInstrDesc DbgValueInst{};
 
   LexicalScopesTest() : Ctx(), Mod("beehives", Ctx) {
     memset(&BeanInst, 0, sizeof(BeanInst));
     BeanInst.Opcode = 1;
     BeanInst.Size = 1;
 
     memset(&DbgValueInst, 0, sizeof(DbgValueInst));
     DbgValueInst.Opcode = TargetOpcode::DBG_VALUE;