[AMDGPU][MC] Added PCK variants of image load/store instructions

[dp]

Summary

See bug 36834: https://bugs.llvm.org/show_bug.cgi?id=36834

These instructions do not support 'd16' modifier.
Other than that documentation describes no differences from usual image load/store opcodes.

[nhaehnle]

lib/Target/AMDGPU/MIMGInstructions.td

88 ↗

(On Diff #139476)

What's actually the problem here? Why doesn't it work to directly have

defm _V1 : MIMG_NoSampler_Src_Helper_Helper <op, asm, VGPR_32, 1, 0, "">;

etc. in the multiclass below?

157 ↗

(On Diff #139476)

Same here.

[dp]

lib/Target/AMDGPU/MIMGInstructions.td

88 ↗

(On Diff #139476)

This is exactly what I did in the initial implementation.
I was surprised to see that in this case TableGen ignores "_V..." suffices specified in MIMG_PckNoSampler.
For example, instead of

IMAGE_LOAD_PCK_V1_V1
IMAGE_LOAD_PCK_V1_V2
IMAGE_LOAD_PCK_V1_V3
IMAGE_LOAD_PCK_V1_V4

IMAGE_LOAD_PCK_V2_V1
IMAGE_LOAD_PCK_V2_V2
IMAGE_LOAD_PCK_V2_V3
IMAGE_LOAD_PCK_V2_V4
...

TableGen generated the following names:

IMAGE_LOAD_PCK_V1
IMAGE_LOAD_PCK_V2
IMAGE_LOAD_PCK_V3
IMAGE_LOAD_PCK_V4

IMAGE_LOAD_PCK_V1
IMAGE_LOAD_PCK_V2
...

As a result I was getting a lot of duplicates and TableGen failed with an error

def "_V1" already defined in this multiclass.

I tried to use

defm NAME # "_V1"
...

but this did not help.

What am I missing here?

[nhaehnle]

lib/Target/AMDGPU/MIMGInstructions.td

88 ↗

(On Diff #139476)

I see. Yeah, there are a bunch of really stupid action-at-a-distance problems with how TableGen resolves names. Have you tried also making the NAME explicit in the defs in MIMG_NoSampler_Src_Helper_Helper, like this:

def NAME # _V1 # suffix : ...;
def NAME # _V2 # suffix : ...;
def NAME # _V4 # suffix : ...;

It is not at all logical, but I think that could help, and in general if you explicitly refer to NAME at all levels of the multiclass nesting I believe it should always work as expected.

[dp]

lib/Target/AMDGPU/MIMGInstructions.td

88 ↗

(On Diff #139476)

Thanks, this cured the problem! But I had to add "NAME" to MIMG_NoSampler_Src_Helper as well to make it work.

[dp]

Corrected as suggested by Nicolai. Thanks!

[dp]

lib/Target/AMDGPU/MIMGInstructions.td

68 ↗

(On Diff #139786)

Without this change MIMG_NoSampler generated names like this:

IMAGE_LOAD_MIP_V1anonymous_2455_V1

Instead of expected

IMAGE_LOAD_MIP_V1_V1

[nhaehnle]

Thanks, LGTM.

lib/Target/AMDGPU/MIMGInstructions.td

68 ↗

(On Diff #139786)

Yeah, good point. For what it's worth, I suspect that using defm "" instead of defm NAME would work as well, but either option is fine.

[artem.tamazov]

LGTM

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

class MIMG_NoSampler_Helper <bits<7> op, string asm,
   let ssamp = 0;
   let D16 = d16;
 }
 
 multiclass MIMG_NoSampler_Src_Helper_Helper <bits<7> op, string asm,
                                              RegisterClass dst_rc,
                                              int channels, bit d16_bit,
                                              string suffix> {
-  def _V1 # suffix : MIMG_NoSampler_Helper <op, asm, dst_rc, VGPR_32, d16_bit,
+  def NAME # _V1 # suffix : MIMG_NoSampler_Helper <op, asm, dst_rc, VGPR_32, d16_bit,
                                                    !if(!eq(channels, 1), "AMDGPU", "")>,
                             MIMG_Mask<asm#"_V1"#suffix, channels>;
-  def _V2 # suffix : MIMG_NoSampler_Helper <op, asm, dst_rc, VReg_64, d16_bit>,
+  def NAME # _V2 # suffix : MIMG_NoSampler_Helper <op, asm, dst_rc, VReg_64, d16_bit>,
                             MIMG_Mask<asm#"_V2"#suffix, channels>;
-  def _V4 # suffix : MIMG_NoSampler_Helper <op, asm, dst_rc, VReg_128, d16_bit>,
+  def NAME # _V4 # suffix : MIMG_NoSampler_Helper <op, asm, dst_rc, VReg_128, d16_bit>,
                             MIMG_Mask<asm#"_V4"#suffix, channels>;
 }
 
 multiclass MIMG_NoSampler_Src_Helper <bits<7> op, string asm,
                                       RegisterClass dst_rc,
                                       int channels> {
-  defm : MIMG_NoSampler_Src_Helper_Helper <op, asm, dst_rc, channels, 0, "">;
+  defm NAME : MIMG_NoSampler_Src_Helper_Helper <op, asm, dst_rc, channels, 0, "">;
 
   let d16 = 1 in {
     let SubtargetPredicate = HasPackedD16VMem in {
-      defm : MIMG_NoSampler_Src_Helper_Helper <op, asm, dst_rc, channels, 1, "_D16">;
+      defm NAME : MIMG_NoSampler_Src_Helper_Helper <op, asm, dst_rc, channels, 1, "_D16">;
     } // End HasPackedD16VMem.
 
     let SubtargetPredicate = HasUnpackedD16VMem, DecoderNamespace = "GFX80_UNPACKED" in {
-      defm : MIMG_NoSampler_Src_Helper_Helper <op, asm, dst_rc, channels, 1, "_D16_gfx80">;
+      defm NAME : MIMG_NoSampler_Src_Helper_Helper <op, asm, dst_rc, channels, 1, "_D16_gfx80">;
     } // End HasUnpackedD16VMem.
   } // End d16 = 1.
 }
 
 multiclass MIMG_NoSampler <bits<7> op, string asm> {
   defm _V1 : MIMG_NoSampler_Src_Helper <op, asm, VGPR_32, 1>;
   defm _V2 : MIMG_NoSampler_Src_Helper <op, asm, VReg_64, 2>;
   defm _V3 : MIMG_NoSampler_Src_Helper <op, asm, VReg_96, 3>;
   defm _V4 : MIMG_NoSampler_Src_Helper <op, asm, VReg_128, 4>;
 }
 
+multiclass MIMG_PckNoSampler <bits<7> op, string asm> {
+  defm NAME # _V1 : MIMG_NoSampler_Src_Helper_Helper <op, asm, VGPR_32, 1, 0, "">;
+  defm NAME # _V2 : MIMG_NoSampler_Src_Helper_Helper <op, asm, VReg_64, 2, 0, "">;
+  defm NAME # _V3 : MIMG_NoSampler_Src_Helper_Helper <op, asm, VReg_96, 3, 0, "">;
+  defm NAME # _V4 : MIMG_NoSampler_Src_Helper_Helper <op, asm, VReg_128, 4, 0, "">;
+}
+
 class MIMG_Store_Helper <bits<7> op, string asm,
                          RegisterClass data_rc,
                          RegisterClass addr_rc,
                          bit d16_bit=0,
                          string dns = ""> : MIMG_Helper <
   (outs),
   (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#" $vdata, $vaddr, $srsrc$dmask$unorm$glc$slc$r128$tfe$lwe$da"#!if(d16_bit, " d16", ""), dns>, MIMGe<op> {
   let ssamp = 0;
   let mayLoad = 0;
   let mayStore = 1;
   let hasSideEffects = 0;
   let hasPostISelHook = 0;
   let DisableWQM = 1;
   let D16 = d16;
 }
 
 multiclass MIMG_Store_Addr_Helper_Helper <bits<7> op, string asm,
                                   RegisterClass data_rc,
                                   int channels, bit d16_bit,
                                   string suffix> {
-  def _V1 # suffix : MIMG_Store_Helper <op, asm, data_rc, VGPR_32, d16_bit,
+  def NAME # _V1 # suffix : MIMG_Store_Helper <op, asm, data_rc, VGPR_32, d16_bit,
                                                !if(!eq(channels, 1), "AMDGPU", "")>,
                             MIMG_Mask<asm#"_V1"#suffix, channels>;
-  def _V2 # suffix : MIMG_Store_Helper <op, asm, data_rc, VReg_64, d16_bit>,
+  def NAME # _V2 # suffix : MIMG_Store_Helper <op, asm, data_rc, VReg_64, d16_bit>,
                             MIMG_Mask<asm#"_V2"#suffix, channels>;
-  def _V4 # suffix : MIMG_Store_Helper <op, asm, data_rc, VReg_128, d16_bit>,
+  def NAME # _V4 # suffix : MIMG_Store_Helper <op, asm, data_rc, VReg_128, d16_bit>,
                             MIMG_Mask<asm#"_V4"#suffix, channels>;
 }
 
 multiclass MIMG_Store_Addr_Helper <bits<7> op, string asm,
                                   RegisterClass data_rc,
                                   int channels> {
-  defm : MIMG_Store_Addr_Helper_Helper <op, asm, data_rc, channels, 0, "">;
+  defm NAME : MIMG_Store_Addr_Helper_Helper <op, asm, data_rc, channels, 0, "">;
 
   let d16 = 1 in {
     let SubtargetPredicate = HasPackedD16VMem in {
-      defm : MIMG_Store_Addr_Helper_Helper <op, asm, data_rc, channels, 1, "_D16">;
+      defm NAME : MIMG_Store_Addr_Helper_Helper <op, asm, data_rc, channels, 1, "_D16">;
     } // End HasPackedD16VMem.
 
     let SubtargetPredicate = HasUnpackedD16VMem, DecoderNamespace = "GFX80_UNPACKED" in {
-      defm : MIMG_Store_Addr_Helper_Helper <op, asm, data_rc, channels, 1, "_D16_gfx80">;
+      defm NAME : MIMG_Store_Addr_Helper_Helper <op, asm, data_rc, channels, 1, "_D16_gfx80">;
     } // End HasUnpackedD16VMem.
   } // End d16 = 1.
 }
 
 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>;
 }
 
+multiclass MIMG_PckStore <bits<7> op, string asm> {
+  defm NAME # _V1 : MIMG_Store_Addr_Helper_Helper <op, asm, VGPR_32, 1, 0, "">;
+  defm NAME # _V2 : MIMG_Store_Addr_Helper_Helper <op, asm, VReg_64, 2, 0, "">;
+  defm NAME # _V3 : MIMG_Store_Addr_Helper_Helper <op, asm, VReg_96, 3, 0, "">;
+  defm NAME # _V4 : MIMG_Store_Addr_Helper_Helper <op, asm, VReg_128, 4, 0, "">;
+}
+
 class MIMG_Atomic_Helper <string asm, RegisterClass data_rc,
                           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",

 multiclass MIMG_Gather_WQM <bits<7> op, string asm> : MIMG_Gather<op, asm, 1>;
 
 //===----------------------------------------------------------------------===//
 // MIMG Instructions
 //===----------------------------------------------------------------------===//
 let SubtargetPredicate = isGCN in {
 defm IMAGE_LOAD : MIMG_NoSampler <0x00000000, "image_load">;
 defm IMAGE_LOAD_MIP : MIMG_NoSampler <0x00000001, "image_load_mip">;
-//def IMAGE_LOAD_PCK : MIMG_NoPattern_ <"image_load_pck", 0x00000002>;
-//def IMAGE_LOAD_PCK_SGN : MIMG_NoPattern_ <"image_load_pck_sgn", 0x00000003>;
-//def IMAGE_LOAD_MIP_PCK : MIMG_NoPattern_ <"image_load_mip_pck", 0x00000004>;
-//def IMAGE_LOAD_MIP_PCK_SGN : MIMG_NoPattern_ <"image_load_mip_pck_sgn", 0x00000005>;
+defm IMAGE_LOAD_PCK : MIMG_PckNoSampler <0x00000002, "image_load_pck">;
+defm IMAGE_LOAD_PCK_SGN : MIMG_PckNoSampler <0x00000003, "image_load_pck_sgn">;
+defm IMAGE_LOAD_MIP_PCK : MIMG_PckNoSampler <0x00000004, "image_load_mip_pck">;
+defm IMAGE_LOAD_MIP_PCK_SGN : MIMG_PckNoSampler <0x00000005, "image_load_mip_pck_sgn">;
 defm IMAGE_STORE : MIMG_Store <0x00000008, "image_store">;
 defm IMAGE_STORE_MIP : MIMG_Store <0x00000009, "image_store_mip">;
-//def IMAGE_STORE_PCK : MIMG_NoPattern_ <"image_store_pck", 0x0000000a>;
-//def IMAGE_STORE_MIP_PCK : MIMG_NoPattern_ <"image_store_mip_pck", 0x0000000b>;
+defm IMAGE_STORE_PCK : MIMG_PckStore <0x0000000a, "image_store_pck">;
+defm IMAGE_STORE_MIP_PCK : MIMG_PckStore <0x0000000b, "image_store_mip_pck">;
 
 let mayLoad = 0, mayStore = 0 in {
 defm IMAGE_GET_RESINFO : MIMG_NoSampler <0x0000000e, "image_get_resinfo">;
 }
 
 defm IMAGE_ATOMIC_SWAP : MIMG_Atomic <mimg<0x0f, 0x10>, "image_atomic_swap">;
 defm IMAGE_ATOMIC_CMPSWAP : MIMG_Atomic <mimg<0x10, 0x11>, "image_atomic_cmpswap", VReg_64, VReg_128>;
 defm IMAGE_ATOMIC_ADD : MIMG_Atomic <mimg<0x11, 0x12>, "image_atomic_add">;

llvm/trunk/test/MC/AMDGPU/mimg.s

 
 image_load v[5:7], v[1:4], s[8:15] dmask:0xf tfe d16
 // NOSICI:   error: d16 modifier is not supported on this GPU
 // NOGFX8_0: error: image data size does not match dmask and tfe
 // GFX8_1:   image_load v[5:7], v[1:4], s[8:15] dmask:0xf tfe d16 ; encoding: [0x00,0x0f,0x01,0xf0,0x01,0x05,0x02,0x80]
 // GFX9:     image_load v[5:7], v[1:4], s[8:15] dmask:0xf tfe d16 ; encoding: [0x00,0x0f,0x01,0xf0,0x01,0x05,0x02,0x80]
 
 //===----------------------------------------------------------------------===//
+// Image Load/Store: PCK variants
+//===----------------------------------------------------------------------===//
+
+image_load_mip_pck v5, v[1:4], s[8:15] dmask:0x1
+// GCN: image_load_mip_pck v5, v[1:4], s[8:15] dmask:0x1 ; encoding: [0x00,0x01,0x10,0xf0,0x01,0x05,0x02,0x00]
+
+image_load_mip_pck v[5:6], v[1:4], s[8:15] dmask:0x3
+// GCN: image_load_mip_pck v[5:6], v[1:4], s[8:15] dmask:0x3 ; encoding: [0x00,0x03,0x10,0xf0,0x01,0x05,0x02,0x00]
+
+image_load_mip_pck v[5:6], v[1:4], s[8:15] dmask:0x1 unorm glc slc tfe lwe da
+// GCN: image_load_mip_pck v[5:6], v[1:4], s[8:15] dmask:0x1 unorm glc slc tfe lwe da ; encoding: [0x00,0x71,0x13,0xf2,0x01,0x05,0x02,0x00]
+
+image_load_mip_pck_sgn v[5:6], v[1:4], s[8:15] dmask:0x5
+// GCN: image_load_mip_pck_sgn v[5:6], v[1:4], s[8:15] dmask:0x5 ; encoding: [0x00,0x05,0x14,0xf0,0x01,0x05,0x02,0x00]
+
+image_load_pck v5, v[1:4], s[8:15] dmask:0x1 glc
+// GCN: image_load_pck v5, v[1:4], s[8:15] dmask:0x1 glc ; encoding: [0x00,0x21,0x08,0xf0,0x01,0x05,0x02,0x00]
+
+image_load_pck_sgn v5, v[1:4], s[8:15] dmask:0x1 lwe
+// GCN: image_load_pck_sgn v5, v[1:4], s[8:15] dmask:0x1 lwe ; encoding: [0x00,0x01,0x0e,0xf0,0x01,0x05,0x02,0x00]
+
+image_load_mip_pck v5, v[1:4], s[8:15] dmask:0x1 d16
+// NOSICI: error: invalid operand for instruction
+// NOVI:   error: invalid operand for instruction
+// NOGFX9: error: invalid operand for instruction
+
+image_store_mip_pck v252, v[2:5], s[12:19] dmask:0x1 unorm
+// GCN: image_store_mip_pck v252, v[2:5], s[12:19] dmask:0x1 unorm ; encoding: [0x00,0x11,0x2c,0xf0,0x02,0xfc,0x03,0x00]
+
+image_store_mip_pck v1, v[2:5], s[12:19] dmask:0x1 unorm glc slc lwe da
+// GCN: image_store_mip_pck v1, v[2:5], s[12:19] dmask:0x1 unorm glc slc lwe da ; encoding: [0x00,0x71,0x2e,0xf2,0x02,0x01,0x03,0x00]
+
+image_store_pck v1, v[2:5], s[12:19] dmask:0x1 unorm da
+// GCN: image_store_pck v1, v[2:5], s[12:19] dmask:0x1 unorm da ; encoding: [0x00,0x51,0x28,0xf0,0x02,0x01,0x03,0x00]
+
+image_store_mip_pck v252, v[2:5], s[12:19] dmask:0x1 d16
+// NOSICI: error: invalid operand for instruction
+// NOVI:   error: invalid operand for instruction
+// NOGFX9: error: invalid operand for instruction
+
+//===----------------------------------------------------------------------===//
 // Image Sample
 //===----------------------------------------------------------------------===//
 
 image_sample  v[193:195], v[237:240], s[28:35], s[4:7] dmask:0x7 unorm
 // GCN: image_sample v[193:195], v[237:240], s[28:35], s[4:7] dmask:0x7 unorm ; encoding: [0x00,0x17,0x80,0xf0,0xed,0xc1,0x27,0x00]
 
 image_sample  v193, v[237:240], s[28:35], s[4:7]
 // GCN: image_sample v193, v[237:240], s[28:35], s[4:7] ; encoding: [0x00,0x00,0x80,0xf0,0xed,0xc1,0x27,0x00]

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

 # GFX81: image_store v[0:1], v4, s[8:15] dmask:0x7 d16 ; encoding: [0x00,0x07,0x20,0xf0,0x04,0x00,0x02,0x80]
 0x00,0x07,0x20,0xf0,0x04,0x00,0x02,0x80
 
 # GFX80: image_store v[0:3], v4, s[8:15] dmask:0xf d16 ; encoding: [0x00,0x0f,0x20,0xf0,0x04,0x00,0x02,0x80]
 # GFX81: image_store v[0:1], v4, s[8:15] dmask:0xf d16 ; encoding: [0x00,0x0f,0x20,0xf0,0x04,0x00,0x02,0x80]
 0x00,0x0f,0x20,0xf0,0x04,0x00,0x02,0x80
 
 #===------------------------------------------------------------------------===#
+# Image load/store: PCK variants
+#===------------------------------------------------------------------------===#
+
+# VI: image_load_mip_pck v5, v1, s[8:15] dmask:0x1 ; encoding: [0x00,0x01,0x10,0xf0,0x01,0x05,0x02,0x00]
+0x00,0x01,0x10,0xf0,0x01,0x05,0x02,0x00
+
+# VI: image_load_mip_pck v[5:6], v1, s[8:15] dmask:0x3 ; encoding: [0x00,0x03,0x10,0xf0,0x01,0x05,0x02,0x00]
+0x00,0x03,0x10,0xf0,0x01,0x05,0x02,0x00
+
+# VI: image_load_mip_pck_sgn v[5:6], v1, s[8:15] dmask:0x5 ; encoding: [0x00,0x05,0x14,0xf0,0x01,0x05,0x02,0x00]
+0x00,0x05,0x14,0xf0,0x01,0x05,0x02,0x00
+
+# VI: image_load_pck v5, v1, s[8:15] dmask:0x1 glc ; encoding: [0x00,0x21,0x08,0xf0,0x01,0x05,0x02,0x00]
+0x00,0x21,0x08,0xf0,0x01,0x05,0x02,0x00
+
+# VI: image_load_pck_sgn v5, v1, s[8:15] dmask:0x1 lwe ; encoding: [0x00,0x01,0x0e,0xf0,0x01,0x05,0x02,0x00]
+0x00,0x01,0x0e,0xf0,0x01,0x05,0x02,0x00
+
+# VI: image_store_mip_pck v252, v2, s[12:19] dmask:0x1 unorm ; encoding: [0x00,0x11,0x2c,0xf0,0x02,0xfc,0x03,0x00]
+0x00,0x11,0x2c,0xf0,0x02,0xfc,0x03,0x00
+
+# VI: image_store_mip_pck v1, v2, s[12:19] dmask:0x1 unorm glc slc lwe da ; encoding: [0x00,0x71,0x2e,0xf2,0x02,0x01,0x03,0x00]
+0x00,0x71,0x2e,0xf2,0x02,0x01,0x03,0x00
+
+# VI: image_store_pck v1, v2, s[12:19] dmask:0x1 unorm da ; encoding: [0x00,0x51,0x28,0xf0,0x02,0x01,0x03,0x00]
+0x00,0x51,0x28,0xf0,0x02,0x01,0x03,0x00
+
+#===------------------------------------------------------------------------===#
 # Image sample
 #===------------------------------------------------------------------------===#
 
 # VI: image_sample v[193:195], v237, s[28:35], s[4:7] dmask:0x7 unorm ; encoding: [0x00,0x17,0x80,0xf0,0xed,0xc1,0x27,0x00]
 0x00,0x17,0x80,0xf0,0xed,0xc1,0x27,0x00
 
 # GFX80: image_sample v[193:194], v237, s[28:35], s[4:7] dmask:0x3 d16 ; encoding: [0x00,0x03,0x80,0xf0,0xed,0xc1,0x27,0x80]
 # GFX81: image_sample v193, v237, s[28:35], s[4:7] dmask:0x3 d16 ; encoding: [0x00,0x03,0x80,0xf0,0xed,0xc1,0x27,0x80]