[PATCH] D22838: AMDGPU/SI: Implement amdgcn image intrinsics

[Tom Stellard via llvm-commits]

tstellarAMD added inline comments.

================
Comment at: include/llvm/IR/IntrinsicsAMDGPU.td:206
@@ -205,2 +205,3 @@
 def int_amdgcn_image_load_mip : AMDGPUImageLoad;
+def int_amdgcn_image_getresinfo : AMDGPUImageLoad;
 
----------------
This should go in a separate patch.  This patch should be only the sampler changes.

================
Comment at: include/llvm/IR/IntrinsicsAMDGPU.td:218
@@ -216,3 +217,3 @@
    llvm_i1_ty],       // slc(imm)
-  []>;
+  [IntrWriteMem]>;
 
----------------
This is an unrelated change.

================
Comment at: include/llvm/IR/IntrinsicsAMDGPU.td:224
@@ +223,3 @@
+class AMDGPUImageSample : Intrinsic <
+    [llvm_v4f32_ty], // vdata(VGPR)
+    [llvm_anyfloat_ty,  // vaddr(VGPR)
----------------
I'm thinking vdata should be llvm_anyfloat_ty, so we can have it return <4 x half> for the d16 operations.  Though it's going to be weird that some <4 x half> values take 4 registers and some only take two.

Another thing I'm not sure of is if image samplers always return floating-point values and never integers.

================
Comment at: include/llvm/IR/IntrinsicsAMDGPU.td:226
@@ +225,3 @@
+    [llvm_anyfloat_ty,  // vaddr(VGPR)
+     llvm_v8i32_ty,     // rsrc(SGPR)
+     llvm_v4i32_ty,      // sampler(SGPR)
----------------
This should be changed to llvm_anyint_ty, so that we can infer the r128 bit.

================
Comment at: include/llvm/IR/IntrinsicsAMDGPU.td:232
@@ +231,3 @@
+     llvm_i1_ty,         // slc(imm)
+     llvm_i1_ty,         // r128(imm)
+     llvm_i1_ty,         // tfe(imm)
----------------
This r128 bit should be dropped.

================
Comment at: lib/Target/AMDGPU/SIInstructions.td:2722-2729
@@ -2698,3 +2721,10 @@
+
+// ======= amdgcn Image Intrinsics ==============
+
+// Image load
 defm : ImageLoadPatterns<int_amdgcn_image_load, "IMAGE_LOAD">;
 defm : ImageLoadPatterns<int_amdgcn_image_load_mip, "IMAGE_LOAD_MIP">;
+def : ImageLoadPattern<int_amdgcn_image_getresinfo, IMAGE_GET_RESINFO_V4_V1, i32>;
+
+// Image store
 defm : ImageStorePatterns<int_amdgcn_image_store, "IMAGE_STORE">;
----------------
These image load/store changes should go in a separate patch.


https://reviews.llvm.org/D22838