-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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.
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
Implicit Exec Mask Operand Missing From MCInstrDesc of Some Opcodes in AMDGPU backend #89830
Comments
@llvm/issue-subscribers-backend-amdgpu Author: Matin Raayai (matinraayai)
The following Opcodes do not have `AMDGPU::EXEC` mask as an implicit operand in their MC Instr Desc:
`GLOBAL_STORE_DWORD_vi`
`BUFFER_LOAD_DWORD_OFFEN_vi`
`GLOBAL_STORE_DWORD_SADDR_vi`
The issue appears when constructing a `llvm::MachineInstr` from a valid `llvm::MCInst`, in the same manner as llvm-exegesis here:
https://github.com/llvm/llvm-project/blob/859de94536425376244940e190e069a09d797737/llvm/tools/llvm-exegesis/lib/Assembler.cpp#L157-L176
For the MIR to be correct, simply adding the explicit operands should be enough, as the implicit operands are automatically added according to the MCInstrDesc when calling the `llvm::BuildMI` here:
https://github.com/llvm/llvm-project/blob/859de94536425376244940e190e069a09d797737/llvm/tools/llvm-exegesis/lib/Assembler.cpp#L157
This method will call the `llvm::MachineInstr` constructor with `NoImplicit` flag set to `false`:
llvm-project/llvm/lib/CodeGen/MachineInstr.cpp Lines 98 to 114 in 859de94
However, when printing the constructed MIR, the implicit exec is nowhere to be seen:
This causes issues when these instructions get verified before running CodeGen passes. CC: @arsenm @kzhuravl |
Why are you trying to run CodeGen passes and MachineVerifier on Real instructions like GLOBAL_STORE_DWORD_vi? In normal CodeGen, all the passes run on Pseudo instructions like GLOBAL_STORE_DWORD, which does have the implicit use of EXEC. These Pseudo instructions are only converted to the corresponding Real instruction for the appropriate subtarget as part of final code emission. |
@jayfoad the main reason is instrumentation. We use MachinePasses to leverage things like the
IMO even if the normal CodeGen won't encounter real opcodes, that doesn't mean MC shouldn't model the implicit operands correctly. This is especially true if someone is using MC to analyze a binary; They shouldn't need to convert the opcode to its pseudo equivalent first just to get the correctly modeled behavior. |
OK. I certainly have no objection to fixing the implicit uses/defs on Real instructions to match the corresponding Pseudo. |
Do the real instructions just not copy the Uses and Defs from the parent pseudo? |
Currently, the tablegen files that generate the instruction definitions in lib/Target/AMDGPU/AMDGPUGenInstrInfo.inc often only include implicit operands for the architecture-independent pseudo instructions, but not for the corresponding real instructions. The missing implicit operands (most prominently: the EXEC mask) do not affect code generation, since that operates on pseudo instructions, but they are problematic when working with real instructions, e.g., as a decoding result from the MC layer. This patch copies the implicit Defs and Uses from pseudo instructions to the corresponding real instructions, so that implicit operands are also defined for real instructions. Addresses issue llvm#89830.
Currently, that's not the case for many instructions. This PR changes that: #93004 @matinraayai : feel free to try the commit and check if it solves your problem. |
Currently, the tablegen files that generate the instruction definitions in lib/Target/AMDGPU/AMDGPUGenInstrInfo.inc often only include implicit operands for the architecture-independent pseudo instructions, but not for the corresponding real instructions. The missing implicit operands (most prominently: the EXEC mask) do not affect code generation, since that operates on pseudo instructions, but they are problematic when working with real instructions, e.g., as a decoding result from the MC layer. This patch copies the implicit Defs and Uses from pseudo instructions to the corresponding real instructions, so that implicit operands are also defined for real instructions. Addresses issue #89830.
Closing this, as the merged PR #93004 should fix the issue. |
The following Opcodes do not have
AMDGPU::EXEC
mask as an implicit operand in their MC Instr Desc:GLOBAL_STORE_DWORD_vi
BUFFER_LOAD_DWORD_OFFEN_vi
GLOBAL_STORE_DWORD_SADDR_vi
The issue appears when constructing a
llvm::MachineInstr
from a validllvm::MCInst
, in the same manner as llvm-exegesis here:llvm-project/llvm/tools/llvm-exegesis/lib/Assembler.cpp
Lines 157 to 176 in 859de94
For the MIR to be correct, simply adding the explicit operands should be enough, as the implicit operands are automatically added according to the MCInstrDesc when calling the
llvm::BuildMI
here:llvm-project/llvm/tools/llvm-exegesis/lib/Assembler.cpp
Line 157 in 859de94
This method will call the
llvm::MachineInstr
constructor withNoImplicit
flag set tofalse
:llvm-project/llvm/lib/CodeGen/MachineInstr.cpp
Lines 98 to 114 in 859de94
However, when printing the constructed MIR, the implicit exec is nowhere to be seen:
This causes issues when these instructions get verified before running CodeGen passes.
CC: @arsenm @kzhuravl
The text was updated successfully, but these errors were encountered: