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

[Changpeng Fang via llvm-commits]

cfang added inline comments.

================
Comment at: include/llvm/IR/IntrinsicsAMDGPU.td:193-198
@@ -192,11 +192,8 @@
 
 class AMDGPUImageLoad : Intrinsic <
   [llvm_v4f32_ty],    // vdata(VGPR)
   [llvm_anyint_ty,    // vaddr(VGPR)
    llvm_v8i32_ty,     // rsrc(SGPR)
    llvm_i32_ty,       // dmask(imm)
-   llvm_i1_ty,        // r128(imm)
-   llvm_i1_ty,        // da(imm)
-   llvm_i1_ty,        // glc(imm)
-   llvm_i1_ty],       // slc(imm)
+   llvm_i32_ty],      // flags (imm)
   [IntrReadMem]>;
----------------
nhaustov wrote:
> nhaehnle wrote:
> > Can we just not do this kind of change, please? I don't see how it improves anything, it's inconsistent with the ISA description which has the flags separate, and it'll require an annoying flag day synchronization with Mesa.
> I agree. Separate flags also play nicely with assembler.
We plan to add and expose d16 bit, so the Intrinsics for Mesa has to be updated anyway. For the future chips, we may add more flag (bit) and all applications (including Mesa) have to be updated every time a new flag is added.  One advantage of using mask parameter is that we don't have to update the application if the new bit is not exposed.    

================
Comment at: include/llvm/IR/IntrinsicsAMDGPU.td:198-201
@@ -197,6 +197,3 @@
    llvm_i32_ty,       // dmask(imm)
-   llvm_i1_ty,        // r128(imm)
-   llvm_i1_ty,        // da(imm)
-   llvm_i1_ty,        // glc(imm)
-   llvm_i1_ty],       // slc(imm)
   [IntrReadMem]>;
----------------
tstellarAMD wrote:
> Changing this intrinsic will break Mesa, we will need to update Mesa before we can commit this.
We will have to add d16 bit! So Mesa will have to be update anyway. 


https://reviews.llvm.org/D22838