MIR: Start diagnosing too many operands on an instruction

Previously this would just assert which was annoying and didn't point to the specific instruction/operand.
llvm · Feb 21, 2022 · 9c7ca51 · 9c7ca51
1 parent ee5580a
commit 9c7ca51
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 8 deletions.
diff --git a/llvm/include/llvm/CodeGen/MachineOperand.h b/llvm/include/llvm/CodeGen/MachineOperand.h
@@ -460,6 +460,16 @@ class MachineOperand {
     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
   //===--------------------------------------------------------------------===//

diff --git a/llvm/lib/CodeGen/MIRParser/MIParser.cpp b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
@@ -1094,11 +1094,23 @@ bool MIParser::parse(MachineInstr *&MI) {
       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)

diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp
@@ -232,16 +232,12 @@ void MachineInstr::addOperand(MachineFunction &MF, const MachineOperand &Op) {
     }
   }
 
-#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();
 

diff --git a/llvm/test/CodeGen/MIR/AMDGPU/extra-imm-operand.mir b/llvm/test/CodeGen/MIR/AMDGPU/extra-imm-operand.mir
@@ -0,0 +1,12 @@
+# 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
+
+...
diff --git a/llvm/test/CodeGen/MIR/AMDGPU/extra-reg-operand.mir b/llvm/test/CodeGen/MIR/AMDGPU/extra-reg-operand.mir
@@ -0,0 +1,12 @@
+# 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
+
+...