This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][NFC] Split MC tests into promoted from VOP2 to VOP3 and only VOP3
ClosedPublic

Authored by mbrkusanin on Oct 18 2022, 2:50 AM.

Diff Detail

Event Timeline

mbrkusanin created this revision.Oct 18 2022, 2:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2022, 2:50 AM
mbrkusanin requested review of this revision.Oct 18 2022, 2:50 AM
mbrkusanin edited reviewers, added: Joe_Nash; removed: jmnash.Oct 18 2022, 2:56 AM
mbrkusanin retitled this revision from [AMDGPU][NFC] Split VOP3 MC tests into promoted from VOP2 to VOP3 and only VOP3 to [AMDGPU][NFC] Split MC tests into promoted from VOP2 to VOP3 and only VOP3.
Joe_Nash accepted this revision.Oct 25 2022, 1:09 PM
This revision is now accepted and ready to land.Oct 25 2022, 1:09 PM
dp added a comment.Oct 25 2022, 1:48 PM

The patch may be useful, but I’m concerned with file naming. I think the naming should be consistent. Right now we have vop3, vop3c, vop3cx and now vop3_from…. Maybe we should think over test naming?

IMO vop3_from_vop2_dpp16 is questionable.

To tell you the truth, I considered splitting VOP3 opcodes into native and promoted parts but was unable to find good file names and abandoned the idea.

Any proposals to make the naming consistent?

In D136148#3883668, @dp wrote:

The patch may be useful, but I’m concerned with file naming. I think the naming should be consistent. Right now we have vop3, vop3c, vop3cx and now vop3_from…. Maybe we should think over test naming?

IMO vop3_from_vop2_dpp16 is questionable.

To tell you the truth, I considered splitting VOP3 opcodes into native and promoted parts but was unable to find good file names and abandoned the idea.

Any proposals to make the naming consistent?

vop3[_{dpp16,dpp8}][_from_{vopc,vopcx,vop1,vop2}] seems ok to me.

dp added a comment.Oct 25 2022, 2:14 PM
In D136148#3883668, @dp wrote:

The patch may be useful, but I’m concerned with file naming. I think the naming should be consistent. Right now we have vop3, vop3c, vop3cx and now vop3_from…. Maybe we should think over test naming?

IMO vop3_from_vop2_dpp16 is questionable.

To tell you the truth, I considered splitting VOP3 opcodes into native and promoted parts but was unable to find good file names and abandoned the idea.

Any proposals to make the naming consistent?

vop3[_{dpp16,dpp8}][_from_{vopc,vopcx,vop1,vop2}] seems ok to me.

Fine with me.

  • Rebase + filename change
dp added inline comments.Oct 27 2022, 10:32 AM
llvm/test/MC/AMDGPU/gfx11_asm_vop3.s
9636

This test was broken, it was not moved to the intended destination file. Could you double-check that the number of tests (and test lines) is identical before and after the splitting?

dp added inline comments.Oct 28 2022, 9:18 AM
llvm/test/MC/AMDGPU/gfx11_asm_vop3_from_vop2.s
806

This is a native VOP3 instruction; it should not be moved. Disassembler tests should be corrected as well.

mbrkusanin updated this revision to Diff 472312.Nov 1 2022, 8:36 AM
mbrkusanin marked 2 inline comments as done.Nov 1 2022, 8:40 AM
mbrkusanin added inline comments.
llvm/test/MC/AMDGPU/gfx11_asm_vop3.s
9636

Good catch. My script did not work properly for the last instruction in file.

llvm/test/MC/AMDGPU/gfx11_asm_vop3_from_vop2.s
806

Moved.

dp accepted this revision.Nov 1 2022, 9:13 AM

LGTM, thanks!

This revision was automatically updated to reflect the committed changes.
mbrkusanin marked 2 inline comments as done.