[llvm] MachineVerifier: Reject extra non-register operands on instructions (PR #73758)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 28 23:15:44 PST 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-backend-msp430
@llvm/pr-subscribers-backend-amdgpu
Author: Matt Arsenault (arsenm)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/73758.diff
7 Files Affected:
- (modified) llvm/lib/CodeGen/MIRParser/MIParser.cpp (+3-12)
- (modified) llvm/lib/CodeGen/MachineInstr.cpp (-4)
- (modified) llvm/lib/CodeGen/MachineVerifier.cpp (+2-2)
- (modified) llvm/lib/Target/MSP430/MSP430RegisterInfo.cpp (+1)
- (removed) llvm/test/CodeGen/MIR/AMDGPU/extra-imm-operand.mir (-13)
- (removed) llvm/test/CodeGen/MIR/AMDGPU/extra-reg-operand.mir (-13)
- (added) llvm/test/MachineVerifier/verify-implicit-def.mir (+30)
``````````diff
diff --git a/llvm/lib/CodeGen/MIRParser/MIParser.cpp b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
index f5d80b2ae27c233..f300b05727f7905 100644
--- a/llvm/lib/CodeGen/MIRParser/MIParser.cpp
+++ b/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;
diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp
index 9e7b4df2576feee..27eae372f8ad764 100644
--- a/llvm/lib/CodeGen/MachineInstr.cpp
+++ b/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.
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index 729dfc67491ee14..aaf9bd740d1379d 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/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);
}
diff --git a/llvm/lib/Target/MSP430/MSP430RegisterInfo.cpp b/llvm/lib/Target/MSP430/MSP430RegisterInfo.cpp
index 4ee40f47b8c19ec..f3900c990469ffc 100644
--- a/llvm/lib/Target/MSP430/MSP430RegisterInfo.cpp
+++ b/llvm/lib/Target/MSP430/MSP430RegisterInfo.cpp
@@ -134,6 +134,7 @@ MSP430RegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II,
MI.setDesc(TII.get(MSP430::MOV16rr));
MI.getOperand(FIOperandNum).ChangeToRegister(BasePtr, false);
+ MI.removeOperand(FIOperandNum + 1);
if (Offset == 0)
return false;
diff --git a/llvm/test/CodeGen/MIR/AMDGPU/extra-imm-operand.mir b/llvm/test/CodeGen/MIR/AMDGPU/extra-imm-operand.mir
deleted file mode 100644
index 65cb32cdd9aebfe..000000000000000
--- a/llvm/test/CodeGen/MIR/AMDGPU/extra-imm-operand.mir
+++ /dev/null
@@ -1,13 +0,0 @@
-# RUN: not llc -march=amdgcn -run-pass=none -o /dev/null %s 2>&1 | \
-# RUN: FileCheck --strict-whitespace %s
-
----
-name: extra_imm_operand
-body: |
- bb.0:
- ; CHECK: [[@LINE+3]]:17: 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
deleted file mode 100644
index d6b2522542a82dd..000000000000000
--- a/llvm/test/CodeGen/MIR/AMDGPU/extra-reg-operand.mir
+++ /dev/null
@@ -1,13 +0,0 @@
-# RUN: not llc -march=amdgcn -run-pass=none -o /dev/null %s 2>&1 | \
-# RUN: FileCheck --strict-whitespace %s
-
----
-name: extra_reg_operand
-body: |
- bb.0:
- ; CHECK: [[@LINE+3]]:17: too many operands for instruction
- ; CHECK-NEXT: {{^}} S_ENDPGM 0, undef $vgpr0
- ; CHECK-NEXT: {{^}} ^
- S_ENDPGM 0, undef $vgpr0
-
-...
diff --git a/llvm/test/MachineVerifier/verify-implicit-def.mir b/llvm/test/MachineVerifier/verify-implicit-def.mir
new file mode 100644
index 000000000000000..8ca6f2dc75c01de
--- /dev/null
+++ b/llvm/test/MachineVerifier/verify-implicit-def.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
+...
+
+
``````````
</details>
https://github.com/llvm/llvm-project/pull/73758
More information about the llvm-commits
mailing list