[mlir][spirv][test] Fork test to allow testing with assertions enabled

[googlewalt]

This way, testing with --debug flag can correctly specify that it requires assertions.

This is a fix for #164098

[llvmbot]

@llvm/pr-subscribers-mlir-spirv

@llvm/pr-subscribers-mlir

Author: Walter Lee (googlewalt)

Changes

This way, testing with --debug flag can correctly specify that it requires assertions.

---

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

2 Files Affected:

• (added) mlir/test/Target/SPIRV/function-decorations-asserts.mlir (+20)
• (modified) mlir/test/Target/SPIRV/function-decorations.mlir (-1)

diff --git a/mlir/test/Target/SPIRV/function-decorations-asserts.mlir b/mlir/test/Target/SPIRV/function-decorations-asserts.mlir
new file mode 100644
index 0000000000000..ebdb9fb1e75e3
--- /dev/null
+++ b/mlir/test/Target/SPIRV/function-decorations-asserts.mlir
@@ -0,0 +1,20 @@
+// REQUIRES: asserts
+// RUN: mlir-translate --no-implicit-module --test-spirv-roundtrip --split-input-file --debug %s | FileCheck %s
+
+spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage], []> {
+    spirv.func @linkage_attr_test_kernel()  "DontInline"  attributes {}  {
+        %uchar_0 = spirv.Constant 0 : i8
+        %ushort_1 = spirv.Constant 1 : i16
+        %uint_0 = spirv.Constant 0 : i32
+        spirv.FunctionCall @outside.func.with.linkage(%uchar_0):(i8) -> ()
+        spirv.Return
+    }
+    // CHECK: linkage_attributes = #spirv.linkage_attributes<linkage_name = "outside.func", linkage_type = <Import>>
+    spirv.func @outside.func.with.linkage(%arg0 : i8) -> () "Pure" attributes {
+      linkage_attributes=#spirv.linkage_attributes<
+        linkage_name="outside.func",
+        linkage_type=<Import>
+      >
+    }
+    spirv.func @inside.func() -> () "Pure" attributes {} {spirv.Return}
+}
diff --git a/mlir/test/Target/SPIRV/function-decorations.mlir b/mlir/test/Target/SPIRV/function-decorations.mlir
index 6098e42f063a2..cf6edaa0a3d5b 100644
--- a/mlir/test/Target/SPIRV/function-decorations.mlir
+++ b/mlir/test/Target/SPIRV/function-decorations.mlir
@@ -1,5 +1,4 @@
 // 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 --debug %s | FileCheck %s
 
 spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage], []> {
     spirv.func @linkage_attr_test_kernel()  "DontInline"  attributes {}  {

[IgWod-IMG]

Apologies for introducing the bug and thank you for fixing it :)

[joker-eph]

Can you elaborate why is this necessary? I can’t tell from the description just now. Thanks!

[joker-eph]

Rereading the description, it is pretty clear actually, sorry.

now I have a concerns though: we don’t rely on the debug output in testing general, I would rather find a test that does not need it.
This value map likely isn’t a debug only feature?

[IgWod-IMG]

As an author of the PR that introduced the --debug test.

My intention wasn't to test the debug output in any way. The bug I fixed was due to a key in the map being invalidated and not being removed. This was not a problem in general as the key was normally not accessed after its invalidation; however, when run with --debug the debug code would do a late iteration over the whole map trying to print those invalidate values resulting in a crash.

I added --debug test as a mean to simply test it doesn't crash anymore and won't regress - I don't check for any debug output.

The map itself is tested quite extensively by all Target lit tests as they all rely on it, this was just a small corner case that would only surface when debug is enabled and externally linked function is present (so far, I only faced those two conditions together in the test in question). So, it’s not about testing a map, but testing a simple code that prints it and is only run in debug. As such, I think having a test with a --debug flag that will ensure it won't crash in the future is an acceptable solution.

Hope that makes sense :)

[joker-eph]

This is a case where I rather not have test than have a "fragile"/"meaningless" test: for testing the map, this test is relying on some deep internal behavior that can change anytime.
Can we just remove this please?

[IgWod-IMG]

Can we just remove this please?

Yes, please see #164383