This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC][GFX11] Correct disassembly of *_e64_dpp opcodes which support op_sel
ClosedPublic

Authored by dp on Jul 13 2022, 6:03 AM.

Details

Summary

These opcodes cannot be disassembled because op_sel operand is missing - it must be added manually.
See bug 56512 for detailed issue analysis.

A similar problem is being addressed in https://reviews.llvm.org/D129084, but it affects different opcodes.

Diff Detail

Event Timeline

dp created this revision.Jul 13 2022, 6:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 6:03 AM
dp requested review of this revision.Jul 13 2022, 6:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 6:03 AM

Overall this is a good patch, thanks for identifying the issue. It will also help towards https://reviews.llvm.org/D129084. See inline.

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
752

This is a very similar to the code in convertVOP3PDPP. Can we de-duplicate it and create a helper function that collects all modifier values?

761

Can we make this generic over instructions that don't have modifiers on certain operands (e.g. DOT2_BF16_BF16) by changing break to continue?

dp added inline comments.Jul 13 2022, 7:54 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
752

Good point, thanks!

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
801

You don't need to calculate constant to put in opsel operand in disassembler, opsel bits are already set in src_modifiers, inst_printer does not access op_sel operand. You can simply use 0.
You only need to add op_sel operand so that other two operands dpp8 and fi are at correct index.

AsmParser uses op_sel operand to parse opsel and then uses that operand to set opsel bit in src_modifiers. Because Srco operands are parsed first, opsel is parsed after all src operands. After opsel bits in src modifiers are set (e.g. after cvtVOP3OpSel or cvtVOP3P) constant value inside opsel operand could be set to 0 since it would not be used any more.

llvm/lib/Target/AMDGPU/VOPInstructions.td
1289

Is this to allow printing of opsel[3] (SISrcMods::DST_OP_SEL in src modifiers)?
btw opsel is not properly parsed for dot8/dot16 (cvtVOP3DPP)

dp updated this revision to Diff 444261.Jul 13 2022, 7:58 AM

Updated as suggested by Joe.

Joe_Nash added inline comments.Jul 13 2022, 8:05 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
801

It is technically true setting op_sel operand to 0 will not affect the disassembler output. However, it will put the operands in an inconsistent state with respect to each other, and for instance when you look at this in the debugger it is very confusing. So while it can save some time/complexity, I think its probably worth it to make all operands correct.

dp added inline comments.Jul 13 2022, 8:21 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
801

Looks like you are right, op_sel=0 would suffice for correct printing. I did not know about that, thanks.

llvm/lib/Target/AMDGPU/VOPInstructions.td
1289

Correct. WIthout this change op_sel is printed without dst bit.

I know that there are some issues with v_dot2_bf16_bf16_e64_dpp, v_dot2_f16_f16_e64_dpp, v_dot2_f32_f16_e64_dpp and v_dot2_f32_bf16_e64_dpp. My tests fail for these opcodes, but I do not know the root cause.

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
801

I checked codegen and it folds opsel into src_modifiers operands and uses 0 for actual opsel_operand.
disassembler sets opsel bits in src_modifiers directly.
inst_printer does not access value inside opsel.
Only AsmParser needs parsed op_sel value for AMDGPUAsmParser::cvt*.
I find it that the simplest option is to use 0 for op_sel, AsmParser would set it to
after it builds actual instruction (usually after AMDGPUAsmParser::cvt*). Also comment about it in a few places since it is not really straightforward how operand is used.

Joe_Nash accepted this revision.Jul 13 2022, 11:32 AM

LGTM, with a note about adding comments.

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
801

The code now is correct and consistent, so I am in favor of landing as is. It would be good to add a comment about some of these details.

llvm/lib/Target/AMDGPU/VOPInstructions.td
1289

Interesting. Then can you update the comment in SIInstrFormats.td:88 to remove the gfx9 only part?

This revision is now accepted and ready to land.Jul 13 2022, 11:32 AM
dp updated this revision to Diff 444598.Jul 14 2022, 4:59 AM

Updated stale comments; added a comment describing how VOP3/VOP3P operand values are used in disassembler.

This revision was landed with ongoing or failed builds.Jul 15 2022, 3:13 AM
This revision was automatically updated to reflect the committed changes.