Page MenuHomePhabricator

[AMDGPU][MC] Correct definition of aliases
ClosedPublic

Authored by dp on Oct 20 2022, 11:10 AM.

Details

Summary

Many aliases are defined incorrectly which affects identification of unsupported opcodes.
See this issue for details.

Diff Detail

Event Timeline

dp created this revision.Oct 20 2022, 11:10 AM
dp requested review of this revision.Oct 20 2022, 11:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 11:10 AM

Overall looks good. Did you observe any instructions that assembled when they shouldn't? It looks like only differences in error message from the tests.

llvm/lib/Target/AMDGPU/VOP2Instructions.td
272

Shouldn't it be set on inst.AsmVariantName? I believe by luck they are both Default in all uses.

dp marked an inline comment as done.Oct 21 2022, 4:15 AM

Did you observe any instructions that assembled when they shouldn't? It looks like only differences in error message from the tests.

The script I used detected some opcodes which are accepted by the assembler, but I'm not sure if they are part of GFX11 ISA:

  • buffer_load_lds_* opcodes (AFAIK support of these depends on specific HW features and I cannot verify if GFX1100 has these features);
  • aliases for *_nc_* opcodes like v_add_i32, v_add_u16, v_add_u32;
  • buffer_wbinvl1 alias.
llvm/lib/Target/AMDGPU/VOP2Instructions.td
272

Using inst is more logical of course. Thanks!

dp updated this revision to Diff 469548.Oct 21 2022, 4:17 AM
dp marked an inline comment as done.

Update as suggested by Joe.

foad added a comment.Oct 21 2022, 5:05 AM

buffer_load_lds_* opcodes (AFAIK support of these depends on specific HW features and I cannot verify if GFX1100 has these features);

These opcodes are new in GFX11, so yes gfx1100 supports them.

This revision is now accepted and ready to land.Oct 21 2022, 8:49 AM
This revision was automatically updated to reflect the committed changes.