This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC] Enabled 21-bit signed offsets for SMEM instructions
ClosedPublic

Authored by dp on May 2 2020, 4:56 AM.

Details

Summary

Added support for 21-bit signed SMEM offsets (GFX9+).

Diff Detail

Event Timeline

dp created this revision.May 2 2020, 4:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2020, 4:57 AM
arsenm added inline comments.May 4 2020, 7:53 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
3427–3441

I think we're missing the same checks in the MachineVerifier. Can you avoid spreading the hardcoded number of bits to multiple places?

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
153

IsBuffer

dp updated this revision to Diff 261878.May 4 2020, 11:16 AM
dp added a reviewer: alex-t.

Corrected issues found by Matt.

dp marked an inline comment as done.May 4 2020, 11:20 AM
dp added inline comments.
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
1305–1307

This code should probably be corrected as well but I do not know if codegen is ready for this change.

arsenm added inline comments.May 4 2020, 12:42 PM
llvm/lib/Target/AMDGPU/MCTargetDesc/SIMCCodeEmitter.cpp
369

This assert is redundant, the getImm below will assert anyway

372–373

Do we really need to explicitly clamp this here?

dp marked 2 inline comments as done.May 4 2020, 2:35 PM
dp added inline comments.
llvm/lib/Target/AMDGPU/MCTargetDesc/SIMCCodeEmitter.cpp
369

My intention was to state explicitly that an immediate is expected. But this is a matter of taste. I’ll remove the assert.

372–373

This is not strictly necessary. However AMD documentation states that unused bits in instruction encoding should be zero. An MC instruction may come directly from codegen, and I’m not sure it does SMEM offset validation. Should I replace this truncation with an assert?

dp updated this revision to Diff 262035.May 5 2020, 2:21 AM

Removed getSMEMOffsetEncoding

arsenm added inline comments.May 5 2020, 7:26 AM
llvm/lib/Target/AMDGPU/MCTargetDesc/SIMCCodeEmitter.cpp
372–373

I would assume codegen would catch this in a verifier, and the assembler verifier would prevent this from getting here, so this should probably be an assert

dp updated this revision to Diff 262114.May 5 2020, 8:05 AM

Restored getSMEMOffsetEncoding; replaced offset truncation with an assert as suggested by Matt.

arsenm accepted this revision.May 5 2020, 1:25 PM

LGTM with assert nit

llvm/lib/Target/AMDGPU/MCTargetDesc/SIMCCodeEmitter.cpp
370

isUINt<20> would be clearer

This revision is now accepted and ready to land.May 5 2020, 1:25 PM
This revision was automatically updated to reflect the committed changes.