This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] gfx11 scalar alu instructions
ClosedPublic

Authored by Joe_Nash on May 12 2022, 12:59 PM.

Details

Reviewers
foad
rampitec
dp
arsenm
Group Reviewers
Restricted Project
Commits
rGd21b9b4946cd: [AMDGPU] gfx11 scalar alu instructions
Summary

MC layer support for SOP(scalar alu operations) including encoding
support for s_delay_alu and s_sendmsg_rtn.

Contributors:
Jay Foad <jay.foad@amd.com>

Patch 7/N for upstreaming of AMDGPU gfx11 architecture.

Depends on D125319

Diff Detail

Event Timeline

Joe_Nash created this revision.May 12 2022, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2022, 12:59 PM
Joe_Nash requested review of this revision.May 12 2022, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2022, 12:59 PM
Joe_Nash added reviewers: foad, rampitec, dp, Restricted Project.May 12 2022, 1:00 PM
rampitec added inline comments.May 12 2022, 2:29 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
6394

All these magic numbers shall go into SIDefines.h.

llvm/test/MC/AMDGPU/gfx11_pretest.s
1 ↗(On Diff #429055)

What does 'pretest' mean in the file name?

Missing test for intrinsics selection?

llvm/include/llvm/IR/IntrinsicsAMDGPU.td
213 ↗(On Diff #429055)

Missing a bunch of attributes (but they’re also the ones missing from every other intrinsic so probably best to just ignore it for now)

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
500

Seems weird that we aren’t guarding each table lookup with subtarget checks

foad added inline comments.May 13 2022, 2:15 AM
llvm/lib/Target/AMDGPU/SOPInstructions.td
1286–1287

This hunk is nothing to do with GFX11, right? Please commit it separately (consider it pre-approved).

1596

One blank line is enough :)

1753

One blank line is enough :)

Joe_Nash marked 3 inline comments as done.May 13 2022, 10:34 AM

Missing test for intrinsics selection?

I've removed the intrinsic from this patch and will add it in a later patch with tests.

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
6394

Good idea for a refactor, can we do this in a later patch?

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
500

It would be possible, but may be redundant with the more fine grained checks done in decodeInstruction via AMDGPUGenDisassemblerTables.inc::checkDecoderPredicate. In any case, I don't think we want that change for this patch.

llvm/lib/Target/AMDGPU/SOPInstructions.td
1286–1287

Did you mean just the comment or also including let hasSideEffects = 1? I've created a patch with both, but it appears to be NFC. Should this have a test? https://reviews.llvm.org/D125569

llvm/test/MC/AMDGPU/gfx11_pretest.s
1 ↗(On Diff #429055)

I shall move the tests to gfx11_asm_scalar.s as a separate file is not needed.

Joe_Nash updated this revision to Diff 429291.May 13 2022, 10:35 AM
Joe_Nash marked an inline comment as done.

remove intrinsic, remove whitespace, split non-gfx11 change

rampitec added inline comments.May 13 2022, 10:36 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
6394

Sure.

arsenm added inline comments.May 16 2022, 2:18 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
6368

Missing tests for all of these parser errors

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
500

Right this is following what's already done but it looks suspicious

Joe_Nash updated this revision to Diff 430050.May 17 2022, 7:16 AM
Joe_Nash marked an inline comment as done.

added parser error tests

arsenm accepted this revision.May 17 2022, 8:23 AM
This revision is now accepted and ready to land.May 17 2022, 8:23 AM
This revision was landed with ongoing or failed builds.May 17 2022, 11:03 AM
This revision was automatically updated to reflect the committed changes.