[AMDGPU] Fix double space in disassembly of ds_gws_sema_* with gds

[foad]

Summary

By setting up the AsmStrings correctly we can remove some special cases
from AMDGPUInstPrinter::printOffset.

[foad]

Unfortunately I can't reliably test this change with FileCheck. It seems to tolerate a double space in the input when I only want to match a single space, even if I use a regexp pattern like {{\ }}.

[arsenm]

llvm/lib/Target/AMDGPU/DSInstructions.td

333

Why is there a $gds operand here if it's not used for printing?

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp

142

Single quotes

[arsenm]

In D90307#2359027, @foad wrote:

Unfortunately I can't reliably test this change with FileCheck. It seems to tolerate a double space in the input when I only want to match a single space, even if I use a regexp pattern like {{\ }}.

There's a -strict-whitespace flag

[foad]

Rebase after adding -strict-whitespace to the tests.

[foad]

Use single quotes.

[foad]

llvm/lib/Target/AMDGPU/DSInstructions.td

333

There's already a TODO for that: https://github.com/llvm/llvm-project/blob/5febc535c4f8e278124cdeb88a4a82d99016944c/llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp#L2501

[rampitec]

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp

142

Are you sure it didn't add an extra space in some other instructions? Are there any other instructions where offset can be the first operand?

[foad]

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp

142

Are you sure it didn't add an extra space in some other instructions?

Well it didn't break any disassembler tests with -strict-whitespace. I did wonder if it might add an extra space at the *end* of some other instruction, that would be hidden because -show-encoding pads with spaces to a fixed column number before adding the "; encoding:" comment. But...

Are there any other instructions where offset can be the first operand?

No, I have looked through the output of tablegen and I can't find any others.

[foad]

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp

142

An alternative fix would be to remove this special handling of OpNo==0 and change the AsmStrings to remove the space between the mnemonic and the $offset operand. I can try that out.

[rampitec]

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp

142

If it works it would be cleaner.

[foad]

Cleaner fix.

[foad]

llvm/lib/Target/AMDGPU/DSInstructions.td

333

D90378

[rampitec]

Thanks, Jay!

llvm/lib/Target/AMDGPU/DSInstructions.td

: DS_Pseudo<opName, (outs), ins, asmOps> {
 
   let has_gds   = 0;
   let gdsValue  = 1;
   let AsmMatchConverter = "cvtDSGds";
 }
 
 class DS_GWS_0D <string opName>
 : DS_GWS<opName,
-  (ins offset:$offset, gds:$gds), "$offset gds"> {
+  (ins offset:$offset, gds:$gds), "${offset}gds"> {
   let hasSideEffects = 1;
 }
 
 class DS_GWS_1D <string opName>
 : DS_GWS<opName,
   (ins VGPR_32:$data0, offset:$offset, gds:$gds), "$data0$offset gds"> {
 
   let has_gws_data0 = 1;

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp

 
 void AMDGPUInstPrinter::printOffset(const MCInst *MI, unsigned OpNo,
                                     const MCSubtargetInfo &STI,
                                     raw_ostream &O) {
   uint16_t Imm = MI->getOperand(OpNo).getImm();
   if (Imm != 0) {
     O << ((OpNo == 0)? "offset:" : " offset:");
     printU16ImmDecOperand(MI, OpNo, O);
+    if (OpNo == 0)
+      O << ' ';
   }
 }
 
 void AMDGPUInstPrinter::printFlatOffset(const MCInst *MI, unsigned OpNo,
                                         const MCSubtargetInfo &STI,
                                         raw_ostream &O) {
   uint16_t Imm = MI->getOperand(OpNo).getImm();
   if (Imm != 0) {

llvm/test/MC/Disassembler/AMDGPU/gfx10_dasm_all.txt

 0x00,0x00,0x6e,0xd8,0xff,0x00,0x00,0x00
 
 # GFX10: ds_gws_sema_br v255 offset:4660 gds     ; encoding: [0x34,0x12,0x6e,0xd8,0xff,0x00,0x00,0x00]
 0x34,0x12,0x6e,0xd8,0xff,0x00,0x00,0x00
 
 # GFX10: ds_gws_sema_br v255 offset:65535 gds    ; encoding: [0xff,0xff,0x6e,0xd8,0xff,0x00,0x00,0x00]
 0xff,0xff,0x6e,0xd8,0xff,0x00,0x00,0x00
 
-# GFX10: ds_gws_sema_p  gds                      ; encoding: [0x00,0x00,0x72,0xd8,0x00,0x00,0x00,0x00]
+# GFX10: ds_gws_sema_p gds                       ; encoding: [0x00,0x00,0x72,0xd8,0x00,0x00,0x00,0x00]
 0x00,0x00,0x72,0xd8,0x00,0x00,0x00,0x00
 
 # GFX10: ds_gws_sema_p offset:4660 gds           ; encoding: [0x34,0x12,0x72,0xd8,0x00,0x00,0x00,0x00]
 0x34,0x12,0x72,0xd8,0x00,0x00,0x00,0x00
 
 # GFX10: ds_gws_sema_p offset:65535 gds          ; encoding: [0xff,0xff,0x72,0xd8,0x00,0x00,0x00,0x00]
 0xff,0xff,0x72,0xd8,0x00,0x00,0x00,0x00
 
-# GFX10: ds_gws_sema_release_all  gds            ; encoding: [0x00,0x00,0x62,0xd8,0x00,0x00,0x00,0x00]
+# GFX10: ds_gws_sema_release_all gds             ; encoding: [0x00,0x00,0x62,0xd8,0x00,0x00,0x00,0x00]
 0x00,0x00,0x62,0xd8,0x00,0x00,0x00,0x00
 
 # GFX10: ds_gws_sema_release_all offset:4660 gds ; encoding: [0x34,0x12,0x62,0xd8,0x00,0x00,0x00,0x00]
 0x34,0x12,0x62,0xd8,0x00,0x00,0x00,0x00
 
 # GFX10: ds_gws_sema_release_all offset:65535 gds ; encoding: [0xff,0xff,0x62,0xd8,0x00,0x00,0x00,0x00]
 0xff,0xff,0x62,0xd8,0x00,0x00,0x00,0x00
 
-# GFX10: ds_gws_sema_v  gds                      ; encoding: [0x00,0x00,0x6a,0xd8,0x00,0x00,0x00,0x00]
+# GFX10: ds_gws_sema_v gds                       ; encoding: [0x00,0x00,0x6a,0xd8,0x00,0x00,0x00,0x00]
 0x00,0x00,0x6a,0xd8,0x00,0x00,0x00,0x00
 
 # GFX10: ds_gws_sema_v offset:4660 gds           ; encoding: [0x34,0x12,0x6a,0xd8,0x00,0x00,0x00,0x00]
 0x34,0x12,0x6a,0xd8,0x00,0x00,0x00,0x00
 
 # GFX10: ds_gws_sema_v offset:65535 gds          ; encoding: [0xff,0xff,0x6a,0xd8,0x00,0x00,0x00,0x00]
 0xff,0xff,0x6a,0xd8,0x00,0x00,0x00,0x00