This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] gfx11 BUF Instructions
ClosedPublic

Authored by Joe_Nash on May 10 2022, 8:31 AM.

Details

Reviewers
dp
Petar.Avramovic
rampitec
Group Reviewers
Restricted Project
Commits
rGc70259405c61: [AMDGPU] gfx11 BUF Instructions
Summary

Includes MachineCode layer support and tests, and MIR tests not requiring
CodeGen pass changes.
Includes a small change in SMInstructions.td to correct encoded bits.

Contributors:
Petar Avramovic <Petar.Avramovic@amd.com>
Dmitry Preobrazhensky <dmitry.preobrazhensky@amd.com>

Depends on D125316

Patch 6/N for upstreaming of AMDGPU gfx11 architecture.

Diff Detail

Unit TestsFailed

Event Timeline

Joe_Nash created this revision.May 10 2022, 8:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 8:31 AM
Joe_Nash requested review of this revision.May 10 2022, 8:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 8:31 AM
Joe_Nash added reviewers: dp, Petar.Avramovic, rampitec, Restricted Project.May 10 2022, 8:32 AM
Petar.Avramovic accepted this revision.May 11 2022, 3:15 AM
Petar.Avramovic added inline comments.
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
1690

line break, lines above are not clang-formated

This revision is now accepted and ready to land.May 11 2022, 3:15 AM
foad added a comment.May 11 2022, 6:32 AM

Includes a small change in SMInstructions.td to correct encoded bits.

Sounds like that should be a separate patch. Why doesn't it affect any MC tests?

Includes a small change in SMInstructions.td to correct encoded bits.

Sounds like that should be a separate patch. Why doesn't it affect any MC tests?

It has to do with SIRegisterInfo.td:214. We now have pregfx11 and postgfx11 version of M0 and SGPR_NULL encoding instead of a common one. So SIRegisterInfo, BUFInstructions and SMInstructions need to be changed in the same commit. Test output doesn't change because the same value is encoded, just referring to a new def name.

foad added a comment.May 11 2022, 7:53 AM

Includes a small change in SMInstructions.td to correct encoded bits.

Sounds like that should be a separate patch. Why doesn't it affect any MC tests?

It has to do with SIRegisterInfo.td:214. We now have pregfx11 and postgfx11 version of M0 and SGPR_NULL encoding instead of a common one. So SIRegisterInfo, BUFInstructions and SMInstructions need to be changed in the same commit. Test output doesn't change because the same value is encoded, just referring to a new def name.

OK. From the commit message I thought you meant it was an unrelated bug fix.

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
214–222

Nit: comments should generally read and be punctuated like full sentences.

Joe_Nash marked 2 inline comments as done.May 11 2022, 11:23 AM
Joe_Nash updated this revision to Diff 428722.May 11 2022, 11:23 AM

Line break in AMDGPUBaseInfo.cpp. Update comment in SIRegisterInfo.td

dp added a comment.May 12 2022, 5:52 AM

Overall looks fine.

A couple of questions:

  • Is dlc a legal modifier for buffer_atomic_* instructions?
  • Why there are no tests for buffer_atomic_* with dlc? There should be at least one negative test with dlc if it is not legal.
foad added a comment.May 12 2022, 5:55 AM
  • Is dlc a legal modifier for buffer_atomic_* instructions?

Yes I am pretty sure it is legal.

In D125319#3508748, @dp wrote:

Overall looks fine.

A couple of questions:

  • Is dlc a legal modifier for buffer_atomic_* instructions?
  • Why there are no tests for buffer_atomic_* with dlc? There should be at least one negative test with dlc if it is not legal.

FYI, it is legal. I am discussing with @Petar.Avramovic how to support it and I will either update this patch or submit a follow up one.

dp accepted this revision.May 13 2022, 3:38 PM

FYI, it is legal. I am discussing with @Petar.Avramovic how to support it and I will either update this patch or submit a follow up one.

I see, thanks. I believe this issue may be fixed by a separate patch.

In D125319#3512893, @dp wrote:

FYI, it is legal. I am discussing with @Petar.Avramovic how to support it and I will either update this patch or submit a follow up one.

I see, thanks. I believe this issue may be fixed by a separate patch.

Thanks, I will land this then.

This revision was landed with ongoing or failed builds.May 16 2022, 7:09 AM
This revision was automatically updated to reflect the committed changes.
llvm/test/MC/Disassembler/AMDGPU/mtbuf_dasm_gfx11.txt