This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC][GFX11] Correct encoding of VOP3/VOP3_DPP v_cmpx* opcodes
ClosedPublic

Authored by dp on Jul 22 2022, 3:58 AM.

Details

Summary

Encode dst=EXEC but allow disassembler accept any dst value.

See bug 56080.

Diff Detail

Event Timeline

dp created this revision.Jul 22 2022, 3:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2022, 3:58 AM
dp requested review of this revision.Jul 22 2022, 3:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2022, 3:58 AM
This revision is now accepted and ready to land.Jul 22 2022, 10:10 AM
critson added inline comments.
llvm/lib/Target/AMDGPU/MCTargetDesc/SIMCCodeEmitter.cpp
342

Could use the following instead to avoid numeric in code?

Encoding |= MRI.getEncodingValue(AMDGPU::EXEC_LO)

dp marked an inline comment as done.Jul 25 2022, 3:38 AM
dp added inline comments.
llvm/lib/Target/AMDGPU/MCTargetDesc/SIMCCodeEmitter.cpp
342

Thanks. I forgot that I could have used getEncodingValue.

dp updated this revision to Diff 447268.Jul 25 2022, 3:39 AM
dp marked an inline comment as done.

Corrected as suggested by reviewers.

Joe_Nash accepted this revision.Jul 25 2022, 6:52 AM

LGTM. But it also occurs to me we could use a warning when disassembling something with non-exec (not 0x7e) in the destination. The point of this was to avoid that potential error, after all, so it would help fixing suspect shaders.

dp added a comment.Jul 26 2022, 6:23 AM

But it also occurs to me we could use a warning when disassembling something with non-exec (not 0x7e) in the destination. The point of this was to avoid that potential error, after all, so it would help fixing suspect shaders.

Added a separate issue.

This revision was landed with ongoing or failed builds.Jul 26 2022, 7:39 AM
This revision was automatically updated to reflect the committed changes.
foad added a comment.Jul 27 2022, 1:16 AM

As mentioned in the bug report, I think this should be done for GFX10 too.

dp added a comment.Jul 27 2022, 6:53 AM

As mentioned in the bug report, I think this should be done for GFX10 too.

I'll create a patch for gfx10 as well, possibly next week.

llvm/test/MC/AMDGPU/gfx11_asm_vopcx.s