Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 30, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89b240c
MachineVerifier: Reject extra non-register operands on instructions
arsenm Nov 29, 2023
046d849
Add comment
arsenm Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 3 additions & 12 deletions llvm/lib/CodeGen/MIRParser/MIParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is "if" missing in the comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It reads OK to me. "Don't check the operands make sense" is a shorter way of saying "Don't check that the operands make sense".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I somewhat misparsed it the first time I read it, and then it looked weird to me every time I looked at it. :)

// improprieties.
for (const auto &Operand : Operands)
MI->addOperand(MF, Operand.Operand);
}

if (assignRegisterTies(*MI, Operands))
return true;
Expand Down
4 changes: 0 additions & 4 deletions llvm/lib/CodeGen/MachineInstr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/CodeGen/MachineVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
3 changes: 3 additions & 0 deletions llvm/lib/Target/MSP430/MSP430RegisterInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
13 changes: 0 additions & 13 deletions llvm/test/CodeGen/MIR/AMDGPU/extra-imm-operand.mir
View file
Open in desktop

This file was deleted.

13 changes: 0 additions & 13 deletions llvm/test/CodeGen/MIR/AMDGPU/extra-reg-operand.mir
View file
Open in desktop

This file was deleted.

30 changes: 30 additions & 0 deletions llvm/test/MachineVerifier/verify-implicit-def.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
...