This is an archive of the discontinued LLVM Phabricator instance.

WIP: [AMDGPU] Don't define _SGPR forms of Real SMEM instructions on GFX10+
Needs RevisionPublic

Authored by foad on Mar 31 2023, 9:06 AM.

Details

Reviewers
dp
arsenm
Group Reviewers
Restricted Project
Summary

On GFX10+ there is no encoding for the _SGPR form of SMEM instructions.
Only _IMM and _SGPR_IMM make sense.

WIP: I have only done GFX10. I need to make the same changes for GFX11.

Diff Detail

Event Timeline

foad created this revision.Mar 31 2023, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 9:06 AM
foad requested review of this revision.Mar 31 2023, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 9:06 AM
foad added inline comments.Mar 31 2023, 9:14 AM
llvm/lib/Target/AMDGPU/SMInstructions.td
978

I am unhappy that there is so much repetition here. Every GFX10 Real instruction has to refine the whole InOperandList just so it can use a different operand type for $offset to get the disassembler to print it in the right way.

I would love to hear ideas for a better way to do this.

llvm/test/MC/AMDGPU/gfx10_asm_smem.s
1365

These assembler changes are intentional and (I think) desirable. Previously these two GFX10 instructions would be assembled to identical binary, but the assembler would print them differently:

s_load_dword s1, s[2:3], 0x0
s_load_dword s1, s[2:3], null

Output of llvm-mc -arch=amdgcn -mcpu=gfx1010 -show-encoding:

	s_load_dword s1, s[2:3], 0x0            ; encoding: [0x41,0x00,0x00,0xf4,0x00,0x00,0x00,0xfa]
	s_load_dword s1, s[2:3], null           ; encoding: [0x41,0x00,0x00,0xf4,0x00,0x00,0x00,0xfa]

I claim that this is wrong, and the assembler should always print them in the same way that a disassembly of the assembled binary would.

Unfortunately this causes a lot of churn in codegen tests.

Joe_Nash added a reviewer: dp.Apr 3 2023, 7:15 AM
Joe_Nash added a subscriber: Joe_Nash.
Joe_Nash added inline comments.
llvm/lib/Target/AMDGPU/SMInstructions.td
978

I don't think there is a way to set one item within the list. You could maybe replace the list with !foreach, but only modifying one item.
Or
SM_Real copies the InOperandList from the pseudo. I assume you want the real and pseduo to be different, because otherwise that operand class change would do more than just affect the disassembler.
You could build the entire InOperandList in SM_Real. So here you would only have to override one type.
But that would repeat the work of building the InOperandList in the pseudo.
It would scale better if we think we would need to do more field overrides in Real instructions only, but it would be more complicated.
Or
I think overriding InOperandList here is pretty reasonable.

llvm/test/MC/AMDGPU/gfx10_asm_smem.s
1365

I'm pretty sure before this patch whenever we printed 'null' it meant SGPR_NULL. So that is a change.
On the other hand, if you asked to assemble an SMem instruction with null (assume offset:0x0) or with offset:0x0 (assume soffset null) it mapped to the same binary. I think its fine to choose a canonical way to print this.

arsenm requested changes to this revision.Aug 17 2023, 3:20 PM

probably needs a rebase

This revision now requires changes to proceed.Aug 17 2023, 3:20 PM