[PATCH] D77232: [mlir][GPUToSPIRV] Make spv.interface_var_abi attribute on arguments either be unspecified on all arguments to use default ABI, or be present on all arguments.

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar added inline comments.


================
Comment at: mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp:346
 
+/// Generate default spv.interface_var_abi attributes for lowering gpu.func to
+/// spv.func if no arguments have the attributes set already.
----------------
antiagainst wrote:
> Generates ... Returns false if any argument already has ABI specified.
Made it populates.


================
Comment at: mlir/test/Conversion/GPUToSPIRV/if.mlir:40
+    gpu.func @kernel_nested_selection(%arg3 : memref<10xf32>,
+                                      %arg4 : memref<10xf32>,
+                                      %arg5 : i1, %arg6 : i1)
----------------
antiagainst wrote:
> Let's revert changes to these tests. They should pass as-is. Applies to following unaffected tests.
This is not really a change, the args are split, thats all. Will remove it anyway.


================
Comment at: mlir/test/Conversion/GPUToSPIRV/simple.mlir:40
+    // CHECK-SAME: spv.entry_point_abi = {local_size = dense<[32, 4, 1]> : vector<3xi32>}
+    gpu.func @basic_module_structure_preset_ABI(
+      %arg0 : f32
----------------
antiagainst wrote:
> I think we can just test the gpu.func itself without the wrapping gpu.module and the corresponding launch op? This way it's clearer regarding the intended use case of attaching these attributes: it gives us a way to use and control the CodeGen side as a stand-alone component. :)
Apparently the `gpu.module` is needed (`gpu.func` must have this as parent). Removed the funcOp that invokes the kernel though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77232/new/

https://reviews.llvm.org/D77232