MachineVerifier: Reject extra non-register operands on instructions (#73758)

We were allowing extra immediate arguments, and only bothering to check
if registers were implicit or not.

Also consolidate extra operand checks in verifier, to make this
testable. We had 3 different places checking if you were trying to build
an instruction with more operands than allowed by the definition. We had
an assertion in addOperand, a direct check in the MIRParser to avoid the
assertion, and the machine verifier checks. Remove the assert and parser
check so the verifier can provide a consistent verification experience,
which will also handle instructions modified in place.

Author: arsenm

llvm/lib/CodeGen/MIRParser/MIParser.cpp

@@ -1175,19 +1175,10 @@ bool MIParser::parse(MachineInstr *&MI) {
   MI = MF.CreateMachineInstr(MCID, DebugLocation, /*NoImplicit=*/true);
   MI->setFlags(Flags);
 
-  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(Operand.Begin, "too many operands for instruction");
-
-      ++NumExplicitOps;
-    }
-
+  // Don't check the operands make sense, let the verifier catch any
+  // improprieties.
+  for (const auto &Operand : Operands)
     MI->addOperand(MF, Operand.Operand);
-  }
 
   if (assignRegisterTies(*MI, Operands))
     return true;

llvm/lib/CodeGen/MachineInstr.cpp

@@ -231,10 +231,6 @@ void MachineInstr::addOperand(MachineFunction &MF, const MachineOperand &Op) {
   // 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((MCID->isVariadic() || OpNo < MCID->getNumOperands() ||
-          Op.isValidExcessOperand()) &&
-         "Trying to add an operand to a machine instr that is already done!");
-
   MachineRegisterInfo *MRI = getRegInfo();
 
   // Determine if the Operands array needs to be reallocated.

llvm/lib/CodeGen/MachineVerifier.cpp

@@ -2126,9 +2126,9 @@ MachineVerifier::visitMachineOperand(const MachineOperand *MO, unsigned MONum) {
       }
     } else if (MO->isReg() && MO->isTied())
       report("Explicit operand should not be tied", MO, MONum);
-  } else {
+  } else if (!MI->isVariadic()) {
     // ARM adds %reg0 operands to indicate predicates. We'll allow that.
-    if (MO->isReg() && !MO->isImplicit() && !MI->isVariadic() && MO->getReg())
+    if (!MO->isValidExcessOperand())
       report("Extra explicit operand on non-variadic instruction", MO, MONum);
   }
 

llvm/lib/Target/MSP430/MSP430RegisterInfo.cpp

@@ -135,6 +135,9 @@ MSP430RegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II,
     MI.setDesc(TII.get(MSP430::MOV16rr));
     MI.getOperand(FIOperandNum).ChangeToRegister(BasePtr, false);
 
+    // Remove the now unused Offset operand.
+    MI.removeOperand(FIOperandNum + 1);
+
     if (Offset == 0)
       return false;
 

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

@@ -0,0 +1,30 @@
+# REQUIRES: amdgpu-registered-target
+# RUN: not --crash llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -run-pass=none -o /dev/null %s 2>&1 | FileCheck %s
+
+---
+name: invalid_reg_sequence
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    ; CHECK:   *** Bad machine code: Too few operands ***
+    IMPLICIT_DEF
+
+    ; FIXME: Error message misleading
+    ; CHECK: *** Bad machine code: Explicit definition must be a register ***
+    IMPLICIT_DEF 0
+
+    ; CHECK: *** Bad machine code: Extra explicit operand on non-variadic instruction ***
+    %1:vgpr_32 = IMPLICIT_DEF 0
+
+    ; CHECK: *** Bad machine code: Extra explicit operand on non-variadic instruction ***
+    ; CHECK: *** Bad machine code: Extra explicit operand on non-variadic instruction ***
+    %2:vgpr_32 = IMPLICIT_DEF 0, 1
+
+    ; CHECK: *** Bad machine code: Extra explicit operand on non-variadic instruction ***
+    %3:vgpr_32 = IMPLICIT_DEF %1
+
+    ; CHECK-NOT: Bad machine code
+    %4:vgpr_32 = IMPLICIT_DEF implicit %1
+...
+
+