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

[Joe Nash via Phabricator via llvm-commits]

Joe_Nash added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SMInstructions.td:978
+  def _IMM_gfx10 : SMEM_Real_Load_gfx10<op, ps#"_IMM"> {
+    let InOperandList = (ins immPs.BaseClass:$sbase, smem_offset_null:$offset, CPol:$cpol);
+  }
----------------
foad wrote:
> 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.
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. 


================
Comment at: llvm/test/MC/AMDGPU/gfx10_asm_smem.s:1365
 s_load_dword s1, s[2:3] glc
-// GFX10: s_load_dword s1, s[2:3], 0x0 glc ; encoding: [0x41,0x00,0x01,0xf4,0x00,0x00,0x00,0xfa]
+// GFX10: s_load_dword s1, s[2:3], null glc ; encoding: [0x41,0x00,0x01,0xf4,0x00,0x00,0x00,0xfa]
 
----------------
foad wrote:
> 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.
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147335/new/

https://reviews.llvm.org/D147335