This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] gfx11 vop3 and inherited vop instructions
ClosedPublic

Authored by Joe_Nash on May 26 2022, 7:44 AM.

Details

Summary

This patch includes MC layer support for VOP3 encoded instructions and generic VOP support
classes.
Some VOP1 and VOP2 instructions which share an encoding with gfx10 and are using
the AssemblerPredicate = isGFX10Plus are also enabled. That predicate
will be changed to isGFX10Only in a later patch.

Patch 15/N for upstreaming of AMDGPU gfx11 architecture.

Depends on D126468

Diff Detail

Event Timeline

Joe_Nash created this revision.May 26 2022, 7:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2022, 7:44 AM
Joe_Nash requested review of this revision.May 26 2022, 7:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2022, 7:44 AM
Joe_Nash added reviewers: rampitec, dp, vpykhtin, foad, Restricted Project.May 26 2022, 7:45 AM
rampitec added inline comments.May 26 2022, 10:17 AM
llvm/lib/Target/AMDGPU/VOPInstructions.td
1119

Indent is misleading, the brace was closed above.

llvm/test/MC/AMDGPU/gfx11_vop123.s
2

You probably need W32 and W64 run lines.

dp added a comment.May 27 2022, 8:56 AM

Overall looks good.

This change also defines VOP1 and VOP2 opcodes so the name of the change is a bit misleading.

Also I noticed that there are instructions defined by this change that have no tests, for example: v_minmax*, v_maxmin*, v_div_scale*, v_sub_nc_i16, v_ashrrev_i64.

llvm/lib/Target/AMDGPU/SIInstrInfo.td
1482

Not used in this change, should go with VOP3P.

llvm/lib/Target/AMDGPU/VOP3Instructions.td
816

What is so special with these opcodes that they are mentioned here? E.g. why v_add_f16 is not in this list?

824

Ditto.

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

This and the following DPP/DPP8 changes do not look relevant to this patch.

961

This sounds rather cryptic to me. Why VOP1 only?

llvm/test/MC/AMDGPU/gfx11_vop123.s
2245

Out of 6400 tests I found only 4 tests with op_sel. Is there a reason for that? Are you planning to switch to sp3 syntax?

llvm/test/MC/Disassembler/AMDGPU/gfx11_dasm_all.txt
19694

This and the following tests are for VOP3P and should be moved elsewhere.

Joe_Nash updated this revision to Diff 433481.Jun 1 2022, 11:51 AM
Joe_Nash marked 7 inline comments as done.

addressed reviews: added W32/W64 asm tests, removed unrelated vop3p and dpp changes

llvm/lib/Target/AMDGPU/VOP3Instructions.td
816

v_add_f16 is significantly different, and will come in a later patch. There is nothing particularly special about those opcodes. I will remove the comment to aid in grepping.

llvm/lib/Target/AMDGPU/VOPInstructions.td
662
961

In vop2, HasModifiers = HasModifiers || HasClamp || HasOmod
In vop1, HasModifiers can be false if HasClamp or HasOmod is true. It is not ideal they are defined this way, but since this class aims to be a common interface for both, it must support both those cases. A later patch could refactor this to be more clear.

llvm/test/MC/AMDGPU/gfx11_vop123.s
2245

op_sel for vop3 is a work in progress. I expect more changes and tests to come at a later point in time.

In D126475#3542688, @dp wrote:

This change also defines VOP1 and VOP2 opcodes so the name of the change is a bit misleading.

This is a side effect of 1) the same encoding for an instruction on GFX10 and GFX11, 2) the AssemblerPredicate = isGFX10Plus on those real instruction definitions in VOP1Instructions.td and 3) Me accidentally putting in extra tests :)
It's easiest to change the commit message at this point however, so I will plan to just do that unless you have an objection.

Also I noticed that there are instructions defined by this change that have no tests, for example: v_minmax*, v_maxmin*, v_div_scale*, v_sub_nc_i16, v_ashrrev_i64.

True, I don't have any test created for these at the moment.

Joe_Nash updated this revision to Diff 433488.Jun 1 2022, 12:09 PM

change commit msg to reflect vop1/vop2

Joe_Nash retitled this revision from [AMDGPU] gfx11 vop3 instructions to [AMDGPU] gfx11 vop3 and inherited vop instructions.Jun 1 2022, 12:11 PM
Joe_Nash edited the summary of this revision. (Show Details)
dp accepted this revision.Jun 1 2022, 1:13 PM

LGTM, though I’d have preferred to add tests for opcodes which have zero test coverage.

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

I see, thanks!

This revision is now accepted and ready to land.Jun 1 2022, 1:13 PM
In D126475#3551325, @dp wrote:

LGTM, though I’d have preferred to add tests for opcodes which have zero test coverage.

Thanks, these will be added in a follow up patch.

This revision was landed with ongoing or failed builds.Jun 2 2022, 11:31 AM
This revision was automatically updated to reflect the committed changes.