This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC][GFX11][NFC] Add missing asm tests for VOP1 instructions
ClosedPublic

Authored by dp on Aug 25 2022, 6:00 AM.

Diff Detail

Event Timeline

dp created this revision.Aug 25 2022, 6:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2022, 6:00 AM
dp requested review of this revision.Aug 25 2022, 6:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2022, 6:00 AM

The added coverage is good, thanks.
A few things I am noticing: The tests seem to be split into 2 alphabetically sorted chunks. Can we just re-sort the file?
There appears to be no coverage for VOP1, VOP2, or VOPC promoted to the VOP3 form for the base instructions (we have coverage for vop3 dpp). Do you plan to cover that?

dp added a comment.Aug 25 2022, 7:46 AM

A few things I am noticing: The tests seem to be split into 2 alphabetically sorted chunks. Can we just re-sort the file?

Sure, I'll re-sort the tests. The script appended missing tests to the end of the file which resulted in two sorted chunks.

Do you think we should keep existing tests if they are good enough? Or could we just replace them with generated tests?

There appears to be no coverage for VOP1, VOP2, or VOPC promoted to the VOP3 form for the base instructions (we have coverage for vop3 dpp). Do you plan to cover that?

Yes, I noticed that. We also have very little coverage for VOPC.DPP, VOPCX.DPP and VOPD. I'm currently analyzing test coverage and will add missing tests for all encodings.

In D132649#3749019, @dp wrote:

A few things I am noticing: The tests seem to be split into 2 alphabetically sorted chunks. Can we just re-sort the file?

Sure, I'll re-sort the tests. The script appended missing tests to the end of the file which resulted in two sorted chunks.

Do you think we should keep existing tests if they are good enough? Or could we just replace them with generated tests?

It seems like the majority of existing tests were generated by a similar script to what you are using. So if you are replacing them with different registers or something, that's fine the coverage will be similar. If there have been one-off tests added for a particular purpose, I wouldn't want to just wipe those out, but there are a small number (zero?) of those.

There appears to be no coverage for VOP1, VOP2, or VOPC promoted to the VOP3 form for the base instructions (we have coverage for vop3 dpp). Do you plan to cover that?

Yes, I noticed that. We also have very little coverage for VOPC.DPP, VOPCX.DPP and VOPD. I'm currently analyzing test coverage and will add missing tests for all encodings.

Great, thanks!

dp updated this revision to Diff 455599.Aug 25 2022, 8:40 AM

Re-generated all tests to make them sorted. This will also simplify future test update after true 16-bit register support is implemented.

Joe_Nash accepted this revision.Aug 25 2022, 8:58 AM
This revision is now accepted and ready to land.Aug 25 2022, 8:58 AM
dp updated this revision to Diff 455857.Aug 26 2022, 3:50 AM

Add a test with forced _e32 suffix.