[PATCH] D133334: [AMDGPU][MC] Warn when disassembling v_cmpx instructions
Dmitry Preobrazhensky via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 6 04:57:16 PDT 2022
dp added inline comments.
================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:717-718
+
+ if (isGFX10Plus() && (Desc.TSFlags & SIInstrFlags::VOP3) &&
+ Desc.hasImplicitDefOfPhysReg(AMDGPU::EXEC) && MI.getNumOperands() >= 5 &&
+ !OpIsExec(MI.getOperand(1)) && !OpIsZero(MI.getOperand(3))) {
----------------
We could reuse `isVCMPX64` here. It may be defined in `AMDGPUBaseInfo` and reused where necessary.
================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:718
+ if (isGFX10Plus() && (Desc.TSFlags & SIInstrFlags::VOP3) &&
+ Desc.hasImplicitDefOfPhysReg(AMDGPU::EXEC) && MI.getNumOperands() >= 5 &&
+ !OpIsExec(MI.getOperand(1)) && !OpIsZero(MI.getOperand(3))) {
----------------
Why is `MI.getNumOperands() >= 5` important?
================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:719
+ Desc.hasImplicitDefOfPhysReg(AMDGPU::EXEC) && MI.getNumOperands() >= 5 &&
+ !OpIsExec(MI.getOperand(1)) && !OpIsZero(MI.getOperand(3))) {
+ outs() << "Warning: unexpected dst value\n";
----------------
This code checks operands of decoded `MCInst`, but decoded instruction does not provide any information about what `dst` was encoded in the original binary representation. To check the `dst` value, we have to work with the original binary code.
================
Comment at: llvm/test/MC/Disassembler/AMDGPU/gfx10_vop3cx.txt:1071
+# GFX10-NOT: Warning: unexpected dst value
# GFX10: v_cmpx_f_f64_e64 exec, v[1:2] ; encoding: [0x7e,0x00,0x30,0xd4,0x7e,0x02,0x02,0x00]
----------------
foad wrote:
> Might be better to do this by changing the RUN lines to use `FileCheck --implicit-check-not="Warning:"`, to catch all unexpected warnings.
I agree. Also, I suggest creating a separate file `gfx10_vop3cx_errs.txt` and add new tests for this feature there. The tests should include regular dst values (0x7e and 0), as well as a couple of values that trigger the warning.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133334/new/
https://reviews.llvm.org/D133334
More information about the llvm-commits
mailing list