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

[Matt Arsenault via llvm-commits]

arsenm 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]>;
----------------
cfang wrote:
> 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.    
The assembler has nothing to do with the intrinsic definition. This isn't changing the MachineInstr's operand structure


https://reviews.llvm.org/D22838