[PATCH] D77232: [mlir][GPUToSPIRV] !!Breaking change!! Make spv.interface_var_abi attribute a required attribute while lowering from gpu.func to spv.func.

[Lei Zhang via Phabricator via llvm-commits]

antiagainst requested changes to this revision.
antiagainst added inline comments.
This revision now requires changes to proceed.


================
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.
----------------
Generates ... Returns false if any argument already has ABI specified.


================
Comment at: mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp:356
+    Optional<spirv::StorageClass> sc;
+    if (funcOp.getArgument(argIndex).getType().isIntOrIndexOrFloat())
+      sc = spirv::StorageClass::StorageBuffer;
----------------
Let's put a comment here to explain why this `if` statement like:

We need to wrap scalar values in a struct to satisfy Vulkan's interface variable requirements. We choose StorageBuffer for holding the struct here.

Otherwise it might be a bit mysterious.


================
Comment at: mlir/test/Conversion/GPUToSPIRV/if.mlir:19
+    gpu.func @kernel_simple_selection(
+      %arg2 : memref<10xf32>
+        {spv.interface_var_abi = {binding = 0 : i32,
----------------
I guess this is changes made when this commit is truly breaking. But now we have a default behavior; we might want to revert these to simplify the tests.


================
Comment at: mlir/test/Conversion/GPUToSPIRV/simple.mlir:39
+    // CHECK:       spv.module Logical GLSL450 {
+    // CHECK-LABEL: spv.func @basic_module_structure_default_ABI
+    // CHECK-SAME: {{%.*}}: f32 {spv.interface_var_abi = {binding = 0 : i32, descriptor_set = 0 : i32, storage_class = 12 : i32{{[}][}]}}
----------------
Instead of checking the default behavior, test the explicit behavior? Also, please make sure the assignment is not sequential as the default behavior.


================
Comment at: mlir/test/mlir-vulkan-runner/addf.mlir:13
+    gpu.func @kernel_add(%arg0 : memref<8xf32>
+                           {spv.interface_var_abi = {binding = 0 : i32,
+                                                     descriptor_set = 0 : i32,
----------------
I think we can omit the changes here; the in-tree Vulkan runner follows the sequential scheme.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77232