[PATCH] D39912: AMDGPU/SI: Implement d16 support for image intrinsics

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/MIMGInstructions.td:39
        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",
+       r128:$r128, tfe:$tfe, lwe:$lwe, da:$da, d16:$d16),
+  asm#" $vdata, $vaddr, $srsrc$dmask$unorm$glc$slc$r128$tfe$lwe$da$d16",
----------------
Since the register class is changing, there shouldn't be an operand for this. This is a separate MI opcode


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:6903
+
+  if (Subtarget->getGeneration() >= SISubtarget::VOLCANIC_ISLANDS) {
+    // Don't adjust if d16 bit is set.
----------------
This should check something more specific than the generation


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:7131-7133
+    int D16Idx = AMDGPU::getNamedOperandIdx(MI.getOpcode(), AMDGPU::OpName::d16);
+    if (D16Idx > 0 && MI.getOperand(D16Idx).getImm() != 0)
+      return;
----------------
There shouldn't be a d16 operand, so this should be unnecessary


================
Comment at: test/CodeGen/AMDGPU/llvm.amdgcn.image.sample.d16.ll:10-11
+; UNPACKED: flat_store_short v[4:5], v0
+; GFX81: flat_store_short v[4:5], v0
+; GFX9: global_store_short v[4:5], v0, off
+define amdgpu_kernel void @image_sample_f16(<4 x float> %coords, <8 x i32> inreg %rsrc, <4 x i32> inreg %sample, half addrspace(1)* %out) {
----------------
Checking exact registers is probably likely to break here


================
Comment at: test/CodeGen/AMDGPU/llvm.amdgcn.image.sample.d16.ll:78
+; GFX9: global_store_short v[4:5], v0, off
+define amdgpu_kernel void @image_sample_c_v4f16(<4 x float> %coords, <8 x i32> inreg %rsrc, <4 x i32> inreg %sample, half addrspace(1)* %out) {
+main_body:
----------------
inreg doesn't do anything with amdgpu_kernel, so you should remove them. 


https://reviews.llvm.org/D39912