MIR: Start diagnosing too many operands on an instruction

[arsenm]

Summary

Previously this would just assert which was annoying and didn't point
to the specific instruction/operand.

[thegameg]

Maybe we can do this in the MachineVerifier too?

[arsenm]

In D120166#3333043, @thegameg wrote:

Maybe we can do this in the MachineVerifier too?

The verifier already checks the number of operands

[arsenm]

9c7ca51b2c9ec648dc69f4891000f2a11ca7698e

llvm/include/llvm/CodeGen/MachineOperand.h

public:
   ///
   /// This refers to reading the register value from before the current
   /// instruction or bundle. Internal bundle reads are not included.
   bool readsReg() const {
     assert(isReg() && "Wrong MachineOperand accessor");
     return !isUndef() && !isInternalRead() && (isUse() || getSubReg());
   }
 
+  /// Return true if this operand can validly be appended to an arbitrary
+  /// operand list. i.e. this behaves like an implicit operand.
+  bool isValidExcessOperand() const {
+    if ((isReg() && isImplicit()) || isRegMask())
+      return true;
+
+    // Debug operands
+    return isMetadata() || isMCSymbol();
+  }
+
   //===--------------------------------------------------------------------===//
   // Mutators for Register Operands
   //===--------------------------------------------------------------------===//
 
   /// Change the register this operand corresponds to.
   ///
   void setReg(Register Reg);
 

llvm/lib/CodeGen/MIRParser/MIParser.cpp

bool MIParser::parse(MachineInstr *&MI) {
 
   const auto &MCID = MF.getSubtarget().getInstrInfo()->get(OpCode);
   if (!MCID.isVariadic()) {
     // FIXME: Move the implicit operand verification to the machine verifier.
     if (verifyImplicitOperands(Operands, MCID))
       return true;
   }
 
-  // TODO: Check for extraneous machine operands.
   MI = MF.CreateMachineInstr(MCID, DebugLocation, /*NoImplicit=*/true);
   MI->setFlags(Flags);
-  for (const auto &Operand : Operands)
+
+  unsigned NumExplicitOps = 0;
+  for (const auto &Operand : Operands) {
+    bool IsImplicitOp = Operand.Operand.isReg() && Operand.Operand.isImplicit();
+    if (!IsImplicitOp) {
+      if (!MCID.isVariadic() && NumExplicitOps >= MCID.getNumOperands() &&
+          !Operand.Operand.isValidExcessOperand())
+        return error("too many operands for instruction");
+
+      ++NumExplicitOps;
+    }
+
     MI->addOperand(MF, Operand.Operand);
+  }
+
   if (assignRegisterTies(*MI, Operands))
     return true;
   if (PreInstrSymbol)
     MI->setPreInstrSymbol(MF, PreInstrSymbol);
   if (PostInstrSymbol)
     MI->setPostInstrSymbol(MF, PostInstrSymbol);
   if (HeapAllocMarker)
     MI->setHeapAllocMarker(MF, HeapAllocMarker);

llvm/lib/CodeGen/MachineInstr.cpp

void MachineInstr::addOperand(MachineFunction &MF, const MachineOperand &Op) {
   bool isImpReg = Op.isReg() && Op.isImplicit();
   if (!isImpReg && !isInlineAsm()) {
     while (OpNo && Operands[OpNo-1].isReg() && Operands[OpNo-1].isImplicit()) {
       --OpNo;
       assert(!Operands[OpNo].isTied() && "Cannot move tied operands");
     }
   }
 
-#ifndef NDEBUG
-  bool isDebugOp = Op.getType() == MachineOperand::MO_Metadata ||
-                   Op.getType() == MachineOperand::MO_MCSymbol;
   // OpNo now points as the desired insertion point.  Unless this is a variadic
   // instruction, only implicit regs are allowed beyond MCID->getNumOperands().
   // RegMask operands go between the explicit and implicit operands.
-  assert((isImpReg || Op.isRegMask() || MCID->isVariadic() ||
-          OpNo < MCID->getNumOperands() || isDebugOp) &&
+  assert((MCID->isVariadic() || OpNo < MCID->getNumOperands() ||
+          Op.isValidExcessOperand()) &&
          "Trying to add an operand to a machine instr that is already done!");
-#endif
 
   MachineRegisterInfo *MRI = getRegInfo();
 
   // Determine if the Operands array needs to be reallocated.
   // Save the old capacity and operand array.
   OperandCapacity OldCap = CapOperands;
   MachineOperand *OldOperands = Operands;
   if (!OldOperands || OldCap.getSize() == getNumOperands()) {

llvm/test/CodeGen/MIR/AMDGPU/extra-imm-operand.mir

+# RUN: not llc -march=amdgcn -run-pass=none -o /dev/null %s 2>&1 | FileCheck %s
+
+---
+name: extra_imm_operand
+body: |
+  bb.0:
+    ; CHECK: [[@LINE+3]]:18: too many operands for instruction
+    ; CHECK-NEXT: S_ENDPGM 0, 0
+    ; CHECK_NEXT:               ^
+    S_ENDPGM 0, 0
+
+...

llvm/test/CodeGen/MIR/AMDGPU/extra-reg-operand.mir

+# RUN: not llc -march=amdgcn -run-pass=none -o /dev/null %s 2>&1 | FileCheck %s
+
+---
+name: extra_reg_operand
+body: |
+  bb.0:
+    ; CHECK: [[@LINE+3]]:29: too many operands for instruction
+    ; S_ENDPGM 0, undef $vgpr0
+    ; CHECK_NEXT:               ^
+    S_ENDPGM 0, undef $vgpr0
+
+...