[AMDGPU][MC] Enabled disassembler for image atomic operations

[dp]

Summary

See bug 35988:

https://bugs.llvm.org/show_bug.cgi?id=35988

[arsenm]

lib/Target/AMDGPU/MIMGInstructions.td

107 ↗

(On Diff #130196)

What does the addr_rc.Size have to do with this?

[dp]

lib/Target/AMDGPU/MIMGInstructions.td

107 ↗

(On Diff #130196)

We cannot disassemble all _V* variants of atomics. They have different address size, bit the size is not encoded.
In other words, all _V* variants are assembled to the same binary code.

So we have to pick one address size and the smallest size looks the safest.

BTW do you know why there are _V1, _V2 and _V4 variants but not V3? According to AMD docs, 3-element addresses are also valid.

[dp]

Sorry, I used Size as if it were ValueType :-)

[arsenm]

lib/Target/AMDGPU/MIMGInstructions.td

107 ↗

(On Diff #130196)

I think that needs a comment.

I don't think V3 is allowed for atomics based on the listed allowed dmask values

[dp]

lib/Target/AMDGPU/MIMGInstructions.td

107 ↗

(On Diff #130196)

AFAIK dmask affects data size rather than address size.

[dp]

Corrected after discussion with Matt:

• code refactored for clarity;
• added comments.

[dp]

I started implementing 64-bit atomics and found out that disassembler should be enabled using 'MIMG_Mask' and 'channels'.
Otherwise we won't be able to reconstruct dst size.

I'm going to update this change one more time...

[artem.tamazov]

Dmitry, please consider the following (excerpt from GCN ISA manual):

It seems that data format and allowed DMASK values are implicitly defined by T#.
The number of address components also defined by T#.

Shall we invent a syntax elements to pass required surface information to the MIMG instructions?

[dp]

This is an interesting idea, but T# is assumed to be in registers. Also note that GFX10 added a 'dim' field to encode address size.

I think we should fix known issues first and have at least 95% tests passed before contemplating any extensions/improvements.

[artem.tamazov]

In D42186#980293, @dp wrote:

This is an interesting idea, but T# is assumed to be in registers. Also note that GFX10 added a 'dim' field to encode address size.

The idea is to pass assumptions about T# to the assembler when MIMG instructions are being handled. Actual value of T# is obviously unavailable.

I think we should fix known issues first and have at least 95% tests passed before contemplating any extensions/improvements.

Absolutely. For now, let's make some (default) assumptions about T#. It would be nice (but not necessary) to see these assumptions implemented programmatically in this patch.

[dp]

Looks like this is the final version. Initial implementation of 64-bit image atomics did not require any changes in this code.
Please review when you have time.

[artem.tamazov]

LGTM.
BTW it would be nice to have a comment explaining the limitations of MIMG support. In short:

• There is a fundamental limitation: only index of the beginning VGPR of address and data is specified/encoded. The number of actually used VGPRs are set implicitly (in the T#) and thus not known during assembling and disassembling.
• This limitation can not be overcame unless violation of the SP3 compatibility requirement
• Also there is a drawback related to CodeGen: the number of used VGPRs can be set to the power of 2 only, but actual number of VGPRs used is more diverse.

llvm/trunk/lib/Target/AMDGPU/MIMGInstructions.td

 multiclass MIMG_Store <bits<7> op, string asm> {
   defm _V1 : MIMG_Store_Addr_Helper <op, asm, VGPR_32, 1>;
   defm _V2 : MIMG_Store_Addr_Helper <op, asm, VReg_64, 2>;
   defm _V3 : MIMG_Store_Addr_Helper <op, asm, VReg_96, 3>;
   defm _V4 : MIMG_Store_Addr_Helper <op, asm, VReg_128, 4>;
 }
 
 class MIMG_Atomic_Helper <string asm, RegisterClass data_rc,
-                          RegisterClass addr_rc> : MIMG_Helper <
+                          RegisterClass addr_rc, string dns="",
+                          bit enableDasm = 0> : MIMG_Helper <
     (outs data_rc:$vdst),
     (ins data_rc:$vdata, addr_rc:$vaddr, SReg_256:$srsrc,
          dmask:$dmask, unorm:$unorm, GLC:$glc, slc:$slc,
          r128:$r128, tfe:$tfe, lwe:$lwe, da:$da),
-    asm#" $vdst, $vaddr, $srsrc$dmask$unorm$glc$slc$r128$tfe$lwe$da"> {
+    asm#" $vdst, $vaddr, $srsrc$dmask$unorm$glc$slc$r128$tfe$lwe$da",
+    !if(enableDasm, dns, "")> {
   let mayLoad = 1;
   let mayStore = 1;
   let hasSideEffects = 1; // FIXME: Remove this
   let hasPostISelHook = 0;
   let DisableWQM = 1;
   let Constraints = "$vdst = $vdata";
   let AsmMatchConverter = "cvtMIMGAtomic";
 }
 
 class MIMG_Atomic_Real_si<mimg op, string name, string asm,
-  RegisterClass data_rc, RegisterClass addr_rc> :
-  MIMG_Atomic_Helper<asm, data_rc, addr_rc>,
+  RegisterClass data_rc, RegisterClass addr_rc, bit enableDasm> :
+  MIMG_Atomic_Helper<asm, data_rc, addr_rc, "SICI", enableDasm>,
   SIMCInstr<name, SIEncodingFamily.SI>,
   MIMGe<op.SI> {
   let isCodeGenOnly = 0;
   let AssemblerPredicates = [isSICI];
-  let DecoderNamespace = "SICI";
   let DisableDecoder = DisableSIDecoder;
 }
 
 class MIMG_Atomic_Real_vi<mimg op, string name, string asm,
-  RegisterClass data_rc, RegisterClass addr_rc> :
-  MIMG_Atomic_Helper<asm, data_rc, addr_rc>,
+  RegisterClass data_rc, RegisterClass addr_rc, bit enableDasm> :
+  MIMG_Atomic_Helper<asm, data_rc, addr_rc, "VI", enableDasm>,
   SIMCInstr<name, SIEncodingFamily.VI>,
   MIMGe<op.VI> {
   let isCodeGenOnly = 0;
   let AssemblerPredicates = [isVI];
-  let DecoderNamespace = "VI";
   let DisableDecoder = DisableVIDecoder;
 }
 
 multiclass MIMG_Atomic_Helper_m <mimg op, string name, string asm,
-                                 RegisterClass data_rc, RegisterClass addr_rc> {
+                                 RegisterClass data_rc,
+                                 RegisterClass addr_rc,
+                                 bit enableDasm = 0> {
   let isPseudo = 1, isCodeGenOnly = 1 in {
     def "" : MIMG_Atomic_Helper<asm, data_rc, addr_rc>,
              SIMCInstr<name, SIEncodingFamily.NONE>;
   }
 
   let ssamp = 0 in {
-    def _si : MIMG_Atomic_Real_si<op, name, asm, data_rc, addr_rc>;
+    def _si : MIMG_Atomic_Real_si<op, name, asm, data_rc, addr_rc, enableDasm>;
 
-    def _vi : MIMG_Atomic_Real_vi<op, name, asm, data_rc, addr_rc>;
+    def _vi : MIMG_Atomic_Real_vi<op, name, asm, data_rc, addr_rc, enableDasm>;
   }
 }
 
 multiclass MIMG_Atomic <mimg op, string asm, RegisterClass data_rc = VGPR_32> {
-  defm _V1 : MIMG_Atomic_Helper_m <op, asm # "_V1", asm, data_rc, VGPR_32>;
+  // _V* variants have different address size, but the size is not encoded.
+  // So only one variant can be disassembled. V1 looks the safest to decode.
+  defm _V1 : MIMG_Atomic_Helper_m <op, asm # "_V1", asm, data_rc, VGPR_32, 1>;
   defm _V2 : MIMG_Atomic_Helper_m <op, asm # "_V2", asm, data_rc, VReg_64>;
   defm _V4 : MIMG_Atomic_Helper_m <op, asm # "_V3", asm, data_rc, VReg_128>;
 }
 
 class MIMG_Sampler_Helper <bits<7> op, string asm,
                            RegisterClass dst_rc,
                            RegisterClass src_rc,
                            bit wqm,

llvm/trunk/test/MC/Disassembler/AMDGPU/mimg_vi.txt

 # RUN: llvm-mc -arch=amdgcn -mcpu=tonga -disassemble -show-encoding < %s | FileCheck -check-prefix=VI %s
 
+#===------------------------------------------------------------------------===#
+# Image load/store
+#===------------------------------------------------------------------------===#
+
 # VI: image_load v[0:3], v4, s[8:15] dmask:0xf unorm ; encoding: [0x00,0x1f,0x00,0xf0,0x04,0x00,0x02,0x00]
 0x00 0x1f 0x00 0xf0 0x04 0x00 0x02 0x00
 
 # VI: image_load v[0:2], v4, s[8:15] dmask:0xe unorm ; encoding: [0x00,0x1e,0x00,0xf0,0x04,0x00,0x02,0x00]
 0x00 0x1e 0x00 0xf0 0x04 0x00 0x02 0x00
 
 # VI: image_load v[0:1], v0, s[0:7] dmask:0x3 unorm ; encoding: [0x00,0x13,0x00,0xf0,0x00,0x00,0x00,0x00]
 0x00 0x13 0x00 0xf0 0x00 0x00 0x00 0x00

 # VI: image_load v254, v0, s[0:7] dmask:0x7 unorm ; encoding: [0x00,0x17,0x00,0xf0,0x00,0xfe,0x00,0x00]
 0x00 0x17 0x00 0xf0 0x00 0xfe 0x00 0x00
 
 # VI: image_load v255, v0, s[0:7] dmask:0x1 unorm ; encoding: [0x00,0x11,0x00,0xf0,0x00,0xff,0x00,0x00]
 0x00 0x11 0x00 0xf0 0x00 0xff 0x00 0x00
 
 # VI: image_load v255, v0, s[0:7] dmask:0x3 unorm ; encoding: [0x00,0x13,0x00,0xf0,0x00,0xff,0x00,0x00]
 0x00 0x13 0x00 0xf0 0x00 0xff 0x00 0x00
+
+#===------------------------------------------------------------------------===#
+# Image atomics
+#===------------------------------------------------------------------------===#
+
+# VI: image_atomic_add v5, v1, s[8:15] dmask:0x1 unorm ; encoding: [0x00,0x11,0x48,0xf0,0x01,0x05,0x02,0x00]
+0x00,0x11,0x48,0xf0,0x01,0x05,0x02,0x00
+
+# VI: image_atomic_add v252, v1, s[8:15] dmask:0x1 unorm ; encoding: [0x00,0x11,0x48,0xf0,0x01,0xfc,0x02,0x00]
+0x00,0x11,0x48,0xf0,0x01,0xfc,0x02,0x00
+
+# VI: image_atomic_add v5, v255, s[8:15] dmask:0x1 unorm ; encoding: [0x00,0x11,0x48,0xf0,0xff,0x05,0x02,0x00]
+0x00,0x11,0x48,0xf0,0xff,0x05,0x02,0x00
+
+# VI: image_atomic_add v5, v1, s[92:99] dmask:0x1 unorm ; encoding: [0x00,0x11,0x48,0xf0,0x01,0x05,0x17,0x00]
+0x00,0x11,0x48,0xf0,0x01,0x05,0x17,0x00
+
+# VI: image_atomic_add v5, v1, s[8:15] dmask:0x1 unorm glc ; encoding: [0x00,0x31,0x48,0xf0,0x01,0x05,0x02,0x00]
+0x00,0x31,0x48,0xf0,0x01,0x05,0x02,0x00
+
+# VI: image_atomic_add v5, v1, s[8:15] dmask:0x1 unorm slc ; encoding: [0x00,0x11,0x48,0xf2,0x01,0x05,0x02,0x00]
+0x00,0x11,0x48,0xf2,0x01,0x05,0x02,0x00
+
+# VI: image_atomic_add v5, v1, s[8:15] dmask:0x1 unorm lwe ; encoding: [0x00,0x11,0x4a,0xf0,0x01,0x05,0x02,0x00]
+0x00,0x11,0x4a,0xf0,0x01,0x05,0x02,0x00
+
+# VI: image_atomic_add v5, v1, s[8:15] dmask:0x1 unorm da ; encoding: [0x00,0x51,0x48,0xf0,0x01,0x05,0x02,0x00]
+0x00,0x51,0x48,0xf0,0x01,0x05,0x02,0x00