[AMDGPU] Search for literals among explicit operands only #130771
Conversation
The code in SIInstrInfo::isOperandLegal crashes in some rare cases when dealing with call pseudos, which are variadic and may thus have more operands than those listed in their instruction descriptions. The excess operands will usually be implicit registers, but may also be the call preserved reg mask. This patch attempts to fix the issue by iterating only through the explicit operands while searching for literal operands.
rovka
requested review from
jayfoad,
arsenm,
ruiling,
mbrkusanin and
mariusz-sikora-at-amd
|
@llvm/pr-subscribers-backend-amdgpu Author: Diana Picus (rovka) ChangesThe code in SIInstrInfo::isOperandLegal crashes in some rare cases when dealing with call pseudos, which are variadic and may thus have more operands than those listed in their instruction descriptions. The excess operands will usually be implicit registers, but may also be the call preserved reg mask. This patch attempts to fix the issue by iterating only through the explicit operands while searching for literal operands. Full diff: https://github.com/llvm/llvm-project/pull/130771.diff 1 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index ae285d069d876..da8533fd5ef5d 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -6063,7 +6063,7 @@ bool SIInstrInfo::isOperandLegal(const MachineInstr &MI, unsigned OpIdx,
}
} else if (!IsInlineConst && !MO->isReg() && isSALU(MI)) {
// There can be at most one literal operand, but it can be repeated.
- for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
+ for (unsigned i = 0, e = MI.getNumExplicitOperands(); i != e; ++i) {
if (i == OpIdx)
continue;
const MachineOperand &Op = MI.getOperand(i);
|
| @@ -0,0 +1,57 @@ | |||
| //===- llvm/unittests/Target/AMDGPU/ExecMayBeModifiedBeforeAnyUse.cpp -----===// | |||
| Register VReg = MRI.createVirtualRegister(&AMDGPU::CCR_SGPR_64RegClass); | ||
| MachineInstr *Callee = | ||
| BuildMI(*BB, E, DL, TII.get(AMDGPU::S_MOV_B64), VReg).addGlobalAddress(F); | ||
| MachineInstr *Call = | ||
| BuildMI(*BB, E, DL, TII.get(AMDGPU::SI_CALL), AMDGPU::SGPR30_SGPR31) | ||
| .addReg(VReg) | ||
| .addImm(0) | ||
| .addRegMask(TRI.getCallPreservedMask(*MF, CallingConv::AMDGPU_Gfx)) | ||
| .addReg(AMDGPU::VGPR0, RegState::Implicit) | ||
| .addReg(AMDGPU::VGPR1, RegState::Implicit); |
There was a problem hiding this comment.
Won't isOperandLegal get called in the verifier? If you take the MIR here, and put it in test/MachineVerifier does it fail?
|
Mirko's fix is already in good shape, let's just merge that one. |
The code in SIInstrInfo::isOperandLegal crashes in some rare cases when dealing with call pseudos, which are variadic and may thus have more operands than those listed in their instruction descriptions. The excess operands will usually be implicit registers, but may also be the call preserved reg mask.
This patch attempts to fix the issue by iterating only through the explicit operands while searching for literal operands.