[llvm] 9c7ca51 - MIR: Start diagnosing too many operands on an instruction

[Matt Arsenault via llvm-commits]

Author: Matt Arsenault
Date: 2022-02-21T10:36:39-05:00
New Revision: 9c7ca51b2c9ec648dc69f4891000f2a11ca7698e

URL: https://github.com/llvm/llvm-project/commit/9c7ca51b2c9ec648dc69f4891000f2a11ca7698e
DIFF: https://github.com/llvm/llvm-project/commit/9c7ca51b2c9ec648dc69f4891000f2a11ca7698e.diff

LOG: 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.

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

Modified: 
    llvm/include/llvm/CodeGen/MachineOperand.h
    llvm/lib/CodeGen/MIRParser/MIParser.cpp
    llvm/lib/CodeGen/MachineInstr.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/CodeGen/MachineOperand.h b/llvm/include/llvm/CodeGen/MachineOperand.h
index eded28183ea25..d9e610a728995 100644
--- 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
index 6477965bdc210..26ae21a9b752e 100644
--- 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
index 85b266afceefe..5e63fecd1bf6a 100644
--- 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
new file mode 100644
index 0000000000000..db484f0798fcf
--- /dev/null
+++ 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
new file mode 100644
index 0000000000000..03a6777167fa6
--- /dev/null
+++ 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
+
+...