This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] gfx11 FLAT Instructions
ClosedPublic

Authored by Joe_Nash on May 19 2022, 11:21 AM.

Details

Reviewers
rampitec
sebastian-ne
Group Reviewers
Restricted Project
Commits
rG835e09c4c3ee: [AMDGPU] gfx11 FLAT Instructions
Summary

MachineCode Support for FLAT type instructions

Contributors:
Sebastian Neubauer <sebastian.neubauer@amd.com>

Patch 12/N for upstreaming of AMDGPU gfx11 architecture.

Depends on D125989

Diff Detail

Event Timeline

Joe_Nash created this revision.May 19 2022, 11:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 11:21 AM
Joe_Nash requested review of this revision.May 19 2022, 11:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 11:21 AM
Joe_Nash added reviewers: rampitec, sebastian-ne, Restricted Project.May 19 2022, 11:22 AM
rampitec added inline comments.May 19 2022, 11:32 AM
llvm/lib/Target/AMDGPU/FLATInstructions.td
64

This is duplicate of the same declarations above.

llvm/test/MC/AMDGPU/flat-gfx11-mnemonic.s
2

It should not be 'not llvm-mc' as long as it is a positive test.

4

Single blank line.

7

Single blank line.

64

Ditto.

llvm/test/MC/AMDGPU/flat-gfx11.s
5 ↗(On Diff #430750)

Ditto 'not'

llvm/test/MC/AMDGPU/gfx11_flat.s
2

There seems to be too much tests doing the same, isn't it?

llvm/test/MC/Disassembler/AMDGPU/flat-gfx11.txt
4

Single blank line.

Joe_Nash updated this revision to Diff 431366.May 23 2022, 7:28 AM
Joe_Nash marked 7 inline comments as done.

remove blank lines, remove negative test from flat-gfx11-mnemonic, remove redundant field

llvm/test/MC/AMDGPU/flat-gfx11.s
5 ↗(On Diff #430750)

This test contains positive tests for gfx11, negative tests for gfx11, and negative tests for previous architectures. I it enables detection of errors with all of those. Is that ok to land as is?

llvm/test/MC/AMDGPU/gfx11_flat.s
2

I have looked at the test cases in all these added test files, and they are all unique.

Looks good to me, thanks for moving out the one negative test.

rampitec added inline comments.May 23 2022, 11:21 AM
llvm/test/MC/AMDGPU/flat-gfx11.s
5 ↗(On Diff #430750)

It has positive tests, but all the run lines prepended with 'not'. This is not right. I believe GFX11 run line shall not have it.

llvm/test/MC/AMDGPU/gfx11_flat.s
2

Test names are misleading. I cannot immediately tell what's the difference between flat-gfx11.s and gfx11_flat.s

sebastian-ne added inline comments.May 24 2022, 1:07 AM
llvm/test/MC/AMDGPU/flat-gfx11.s
5 ↗(On Diff #430750)

There are other files that have positive and negative tests in them where all run lines are prepended with 'not', e.g. sop2.s, vop1.s, vop3.s and trap.s, so this is not a new pattern.

A negative gfx11 test in this file is e.g.

flat_load_b32 v1, v[3:4], s[0:1]
// GFX11-ERR: error: invalid operand for instruction
rampitec added inline comments.May 24 2022, 9:12 AM
llvm/test/MC/AMDGPU/flat-gfx11.s
5 ↗(On Diff #430750)

This is yet another problem. You have GFX11-ERR labels in this file, but no run line with this check. Move negative checks into gfx11-err.s and remove the 'not'. As a side note, it is unfortunate that smem.s does that.

kosarev added inline comments.May 24 2022, 9:34 AM
llvm/test/MC/AMDGPU/flat-gfx11.s
5 ↗(On Diff #430750)

For a large change like this, maybe it wouldn't be a terrible crime to follow the current conventions and address that already-existing problem separately?

rampitec added inline comments.May 24 2022, 9:47 AM
llvm/test/MC/AMDGPU/flat-gfx11.s
5 ↗(On Diff #430750)

It's a relatively easy change?

Joe_Nash added inline comments.May 24 2022, 10:42 AM
llvm/test/MC/AMDGPU/flat-gfx11.s
5 ↗(On Diff #430750)

We can improve something here, I will discuss with @sebastian-ne and update when the course is clear.

Joe_Nash updated this revision to Diff 431732.May 24 2022, 11:10 AM

Refactored test files, separated negative tests, and corrected some tests for the number of bits in the offset field

This revision is now accepted and ready to land.May 24 2022, 11:14 AM
This revision was landed with ongoing or failed builds.May 25 2022, 12:57 PM
This revision was automatically updated to reflect the committed changes.
Joe_Nash marked an inline comment as done.