[mlir][spirv] Add spirv validation for module.mlir target test

[IgWod-IMG]

Creating this patch as an example on using the new mlir-translate flag. Eventually all tests will be updated to validate SPIR-V modules.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-spirv

Author: Igor Wodiany (IgWod-IMG)

Changes

Creating this patch as an example on using the new mlir-translate flag. Eventually all tests will be updated to validate SPIR-V modules.

---

Full diff: https://github.com/llvm/llvm-project/pull/153227.diff

1 Files Affected:

• (modified) mlir/test/Target/SPIRV/module.mlir (+13-5)

diff --git a/mlir/test/Target/SPIRV/module.mlir b/mlir/test/Target/SPIRV/module.mlir
index dcdcab8097e41..e610001a3a71f 100644
--- a/mlir/test/Target/SPIRV/module.mlir
+++ b/mlir/test/Target/SPIRV/module.mlir
@@ -1,21 +1,29 @@
-// RUN: mlir-translate -no-implicit-module -test-spirv-roundtrip -split-input-file %s | FileCheck %s
+// RUN: mlir-translate --no-implicit-module --test-spirv-roundtrip --split-input-file %s | FileCheck %s
+
+// REQUIRES: shell
+// RUN: %if spirv-tools %{ rm -rf %t %}
+// RUN: %if spirv-tools %{ mkdir %t && mlir-translate --no-implicit-module --serialize-spirv \ %}
+// RUN: %if spirv-tools %{ --split-input-file --spirv-save-validation-files-with-prefix=%t/module %s \ %}
+// RUN: %if spirv-tools %{ && ls %t/module* | xargs -I{} bash -c 'spirv-val {}' %}
 
 // CHECK:      spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader], []> {
 // CHECK-NEXT:   spirv.func @foo() "Inline" {
 // CHECK-NEXT:     spirv.Return
 // CHECK-NEXT:   }
+// CHECK-NEXT:   spirv.EntryPoint "Vertex" @foo
 // CHECK-NEXT: }
 
 spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader], []> {
   spirv.func @foo() -> () "Inline" {
      spirv.Return
   }
+  spirv.EntryPoint "Vertex" @foo
 }
 
 // -----
 
 // CHECK: v1.5
-spirv.module Logical GLSL450 requires #spirv.vce<v1.5, [Shader], []> {
+spirv.module Logical GLSL450 requires #spirv.vce<v1.5, [Shader, Linkage], []> {
 }
 
 // -----
@@ -26,13 +34,13 @@ spirv.module Logical GLSL450 requires #spirv.vce<v1.6, [Shader, Linkage], []> {
 
 // -----
 
-// CHECK: [Shader, Float16]
-spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Float16], []> {
+// CHECK: [Shader, Float16, Linkage]
+spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Float16, Linkage], []> {
 }
 
 // -----
 
 // CHECK: [SPV_KHR_float_controls, SPV_KHR_subgroup_vote]
-spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader], [SPV_KHR_float_controls, SPV_KHR_subgroup_vote]> {
+spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage], [SPV_KHR_float_controls, SPV_KHR_subgroup_vote]> {
 }
 

[IgWod-IMG]

One more thing. Do we want to use the xargs approach (or similar) across the tests or should we try first to teach spirv-val to consume multiple files / directories?

I'll also update the SPIR-V dialect documentation once this PR is merged to make it clear in the contributions section that Target tests need to be validated.

CC @davidegrohmann @mahabadm

[kuhar]

Nice to see it caught legitimate issues already

[kuhar]

Can we specify the extension?

Also, why not xargs -I{} spirv-val {}?

[IgWod-IMG]

What do you mean by the extension?

Also, why not xargs -I{} spirv-val {}?

Good point, I'll simplfy it. The current code was an evolution of various attempts.

[kuhar]

File extension like .spv

[IgWod-IMG]

To have an extension I need to update the logic that writes files. I created a seperate PR #153440 for that.

[kuhar]

Do we want to use the xargs approach (or similar) across the tests or should we try first to teach spirv-val to consume multiple files / directories?

I think it's fine to have xargs / for loop in a single test like this one, but I wouldn't use it elsewhere until spirv-val learns to take directories.

[IgWod-IMG]

I had to replace:

// RUN: %if spirv-tools %{ mkdir %t && mlir-translate --no-implicit-module --serialize-spirv \ %}
// RUN: %if spirv-tools %{ --split-input-file --spirv-save-validation-files-with-prefix=%t/module %s \ %}
// RUN: %if spirv-tools %{ && ls %t/module* | xargs -I{} spirv-val {} %}

with the code below, where every command occupies a single RUN line. This makes mlir-translate to go over the ~80 character limit, however there seem to be issues when conditional substitution and line splitting (\) are used together - shell parser keep tripping over (e.g., shell parser error on RUN: at line 7: && ls /home/user/build-debug/tools/mlir/test/Target/SPIRV/Output/module.mlir.tmp/module*.spv | xargs -I{}/home/user/build-debug/./bin/spirv-val {}).

// RUN: %if spirv-tools %{ mkdir %t %}
// RUN: %if spirv-tools %{ mlir-translate --no-implicit-module --serialize-spirv --split-input-file --spirv-save-validation-files-with-prefix=%t/module %s %}
// RUN: %if spirv-tools %{ ls %t/module*.spv | xargs -I{} spirv-val {} %}