[Mlir-commits] [mlir] [mlir][spirv] Fix int type declaration duplication when serializing (PR #143108)
Davide Grohmann
llvmlistbot at llvm.org
Wed Jun 11 08:08:06 PDT 2025
https://github.com/davidegrohmann updated https://github.com/llvm/llvm-project/pull/143108
>From fdfac9146abe9590745f3f7d6cc572899e19cf90 Mon Sep 17 00:00:00 2001
From: Davide Grohmann <davide.grohmann at arm.com>
Date: Thu, 5 Jun 2025 14:50:20 +0200
Subject: [PATCH 1/4] [mlir][spirv] Fix int type declaration duplication when
serializing
At the MLIR level unsigned integer and signless integers are different
types. Indeed when looking up the two types in type definition cache
they do not match.
Hence when translating a SPIR-V module which contains both usign and
signless integers will contain the same type declaration twice
(something like OpTypeInt 32 0) which is not permitted in SPIR-V and
such generated modules fail validation.
This patch solves the problem by mapping unisgned integer types to
singless integer types before looking up in the type definition cache.
Signed-off-by: Davide Grohmann <davide.grohmann at arm.com>
Change-Id: I9b5ecc72049d2642308b70acc335b6dc6bf6b5be
---
mlir/lib/Target/SPIRV/Serialization/Serializer.cpp | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp b/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
index 15e06616f4492..c9b2a01091460 100644
--- a/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
+++ b/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
@@ -446,6 +446,19 @@ LogicalResult Serializer::processType(Location loc, Type type,
LogicalResult
Serializer::processTypeImpl(Location loc, Type type, uint32_t &typeID,
SetVector<StringRef> &serializationCtx) {
+
+ // Map unisgned integer types to singless integer types
+ // This is needed otherwise the generated spirv assembly will contain
+ // twice a type declaration (like OpTypeInt 32 0) which is no permitted and
+ // such module could no pass validation. Indeed at MLIR level the two types
+ // are different and lookup in the cache below fails.
+ // Note: This convertion needs to happen here before the type is looked up in
+ // the cache
+ if (type.isUnsignedInteger()) {
+ type = IntegerType::get(loc->getContext(), type.getIntOrFloatBitWidth(),
+ IntegerType::SignednessSemantics::Signless);
+ }
+
typeID = getTypeID(type);
if (typeID)
return success();
>From 3fa2991daabb71ac11f98b2872817f3c00b5f2c8 Mon Sep 17 00:00:00 2001
From: Davide Grohmann <davide.grohmann at arm.com>
Date: Mon, 9 Jun 2025 10:07:20 +0200
Subject: [PATCH 2/4] Resolve review comments
Also add capabilities and entry point to allow the spirv assembly
generated from the constants.mlir test to pass validation
Signed-off-by: Davide Grohmann <davide.grohmann at arm.com>
Change-Id: I22df9051de66a3dcb11d70bb95d06801aea62942
---
mlir/lib/Target/SPIRV/Serialization/Serializer.cpp | 10 +++++-----
mlir/test/Target/SPIRV/constant.mlir | 6 ++++--
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp b/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
index c9b2a01091460..4f32237cc42e8 100644
--- a/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
+++ b/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
@@ -447,13 +447,13 @@ LogicalResult
Serializer::processTypeImpl(Location loc, Type type, uint32_t &typeID,
SetVector<StringRef> &serializationCtx) {
- // Map unisgned integer types to singless integer types
+ // Map unsigned integer types to singless integer types.
// This is needed otherwise the generated spirv assembly will contain
// twice a type declaration (like OpTypeInt 32 0) which is no permitted and
- // such module could no pass validation. Indeed at MLIR level the two types
- // are different and lookup in the cache below fails.
- // Note: This convertion needs to happen here before the type is looked up in
- // the cache
+ // such module fails validation. Indeed at MLIR level the two types are
+ // different and lookup in the cache below misses.
+ // Note: This conversion needs to happen here before the type is looked up in
+ // the cache.
if (type.isUnsignedInteger()) {
type = IntegerType::get(loc->getContext(), type.getIntOrFloatBitWidth(),
IntegerType::SignednessSemantics::Signless);
diff --git a/mlir/test/Target/SPIRV/constant.mlir b/mlir/test/Target/SPIRV/constant.mlir
index f3950214a7f05..516aff04667f5 100644
--- a/mlir/test/Target/SPIRV/constant.mlir
+++ b/mlir/test/Target/SPIRV/constant.mlir
@@ -1,6 +1,6 @@
// RUN: mlir-translate -no-implicit-module -test-spirv-roundtrip %s | FileCheck %s
-spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader], []> {
+spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Int64, Int16, Int8, Float64, Float16], []> {
// CHECK-LABEL: @bool_const
spirv.func @bool_const() -> () "None" {
// CHECK: spirv.Constant true
@@ -277,4 +277,6 @@ spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader], []> {
%signed_minus_one = spirv.Constant -1 : si16
spirv.ReturnValue %signed_minus_one : si16
}
-}
+
+ spirv.EntryPoint "GLCompute" @bool_const
+ }
>From cb60131e33e6ef924d112b77e0f84fb704dd561a Mon Sep 17 00:00:00 2001
From: Davide Grohmann <davide.grohmann at arm.com>
Date: Tue, 10 Jun 2025 09:38:14 +0200
Subject: [PATCH 3/4] Add optional SPIR-V validation in constant.mlir Target
test
SPIR-V validation is run if spirv-tools are available otherwise
skipped.
Signed-off-by: Davide Grohmann <davide.grohmann at arm.com>
Change-Id: Id0af7c983c4bd6596a4837812f22ced3c7616177
---
mlir/test/Target/SPIRV/constant.mlir | 1 +
1 file changed, 1 insertion(+)
diff --git a/mlir/test/Target/SPIRV/constant.mlir b/mlir/test/Target/SPIRV/constant.mlir
index 516aff04667f5..72737b0b794b5 100644
--- a/mlir/test/Target/SPIRV/constant.mlir
+++ b/mlir/test/Target/SPIRV/constant.mlir
@@ -1,4 +1,5 @@
// RUN: mlir-translate -no-implicit-module -test-spirv-roundtrip %s | FileCheck %s
+// RUN: %if spirv-tools %{ mlir-translate -no-implicit-module -serialize-spirv %s | spirv-val %}
spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Int64, Int16, Int8, Float64, Float16], []> {
// CHECK-LABEL: @bool_const
>From afab0c51bac123d3b47ff0be5be45abc102a0a97 Mon Sep 17 00:00:00 2001
From: Davide Grohmann <davide.grohmann at arm.com>
Date: Wed, 11 Jun 2025 16:52:07 +0200
Subject: [PATCH 4/4] Enable spirv-tools integration when running mlir Target
tests
Also fix validation failures caused by a rebase
Signed-off-by: Davide Grohmann <davide.grohmann at arm.com>
Change-Id: I221881875c77a30a4df4bd9b15288ca10e9c8e9c
---
mlir/test/CMakeLists.txt | 6 ++++++
mlir/test/Target/SPIRV/constant.mlir | 2 +-
mlir/test/lit.cfg.py | 1 +
mlir/test/lit.local.cfg | 7 +++++++
mlir/test/lit.site.cfg.py.in | 4 +++-
5 files changed, 18 insertions(+), 2 deletions(-)
create mode 100644 mlir/test/lit.local.cfg
diff --git a/mlir/test/CMakeLists.txt b/mlir/test/CMakeLists.txt
index ac8b44f53aebf..89568e7766ae5 100644
--- a/mlir/test/CMakeLists.txt
+++ b/mlir/test/CMakeLists.txt
@@ -68,6 +68,7 @@ endif()
llvm_canonicalize_cmake_booleans(
LLVM_BUILD_EXAMPLES
LLVM_HAS_NVPTX_TARGET
+ LLVM_INCLUDE_SPIRV_TOOLS_TESTS
MLIR_ENABLE_BINDINGS_PYTHON
MLIR_ENABLE_CUDA_RUNNER
MLIR_ENABLE_ROCM_CONVERSIONS
@@ -217,6 +218,11 @@ if(MLIR_ENABLE_BINDINGS_PYTHON)
)
endif()
+if (LLVM_INCLUDE_SPIRV_TOOLS_TESTS)
+ list(APPEND MLIR_TEST_DEPENDS spirv-as)
+ list(APPEND MLIR_TEST_DEPENDS spirv-val)
+endif()
+
# This target can be used to just build the dependencies
# for the check-mlir target without executing the tests.
# This is useful for bots when splitting the build step
diff --git a/mlir/test/Target/SPIRV/constant.mlir b/mlir/test/Target/SPIRV/constant.mlir
index 2269b08bf397a..50d9b09ee0042 100644
--- a/mlir/test/Target/SPIRV/constant.mlir
+++ b/mlir/test/Target/SPIRV/constant.mlir
@@ -1,7 +1,7 @@
// RUN: mlir-translate --no-implicit-module --test-spirv-roundtrip %s | FileCheck %s
// RUN: %if spirv-tools %{ mlir-translate -no-implicit-module -serialize-spirv %s | spirv-val %}
-spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Int64, Int16, Int8, Float64, Float16], []> {
+spirv.module Logical Vulkan requires #spirv.vce<v1.3, [VulkanMemoryModel, Shader, Int64, Int16, Int8, Float64, Float16, CooperativeMatrixKHR], [SPV_KHR_vulkan_memory_model, SPV_KHR_cooperative_matrix]> {
// CHECK-LABEL: @bool_const
spirv.func @bool_const() -> () "None" {
// CHECK: spirv.Constant true
diff --git a/mlir/test/lit.cfg.py b/mlir/test/lit.cfg.py
index 9b5cadd62befc..a6f1ac0d568f4 100644
--- a/mlir/test/lit.cfg.py
+++ b/mlir/test/lit.cfg.py
@@ -332,6 +332,7 @@ def find_real_python_interpreter():
else:
config.available_features.add("noasserts")
+config.targets = frozenset(config.targets_to_build.split())
def have_host_jit_feature_support(feature_name):
mlir_runner_exe = lit.util.which("mlir-runner", config.mlir_tools_dir)
diff --git a/mlir/test/lit.local.cfg b/mlir/test/lit.local.cfg
new file mode 100644
index 0000000000000..167c454db5184
--- /dev/null
+++ b/mlir/test/lit.local.cfg
@@ -0,0 +1,7 @@
+if not "SPIRV" in config.root.targets:
+ config.unsupported = True
+
+if config.spirv_tools_tests:
+ config.available_features.add("spirv-tools")
+ config.substitutions.append(("spirv-as", os.path.join(config.llvm_tools_dir, "spirv-as")))
+ config.substitutions.append(("spirv-val", os.path.join(config.llvm_tools_dir, "spirv-val")))
diff --git a/mlir/test/lit.site.cfg.py.in b/mlir/test/lit.site.cfg.py.in
index 132aabe135940..77f24e0f29b09 100644
--- a/mlir/test/lit.site.cfg.py.in
+++ b/mlir/test/lit.site.cfg.py.in
@@ -5,6 +5,8 @@ import sys
config.target_triple = "@LLVM_TARGET_TRIPLE@"
config.llvm_src_root = "@LLVM_SOURCE_DIR@"
config.llvm_tools_dir = lit_config.substitute("@LLVM_TOOLS_DIR@")
+config.spirv_tools_tests = @LLVM_INCLUDE_SPIRV_TOOLS_TESTS@
+config.targets_to_build = "@TARGETS_TO_BUILD@"
config.llvm_shlib_ext = "@SHLIBEXT@"
config.llvm_shlib_dir = lit_config.substitute(path(r"@SHLIBDIR@"))
config.python_executable = "@Python3_EXECUTABLE@"
@@ -41,7 +43,7 @@ config.mlir_run_amx_tests = @MLIR_RUN_AMX_TESTS@
config.mlir_run_arm_sve_tests = @MLIR_RUN_ARM_SVE_TESTS@
# This is a workaround for the fact that LIT's:
# %if <cond>
-# requires <cond> to be in the set of available features.
+# requires <cond> to be in the set of available features.
# TODO: Update LIT's TestRunner so that this is not required.
if config.mlir_run_arm_sve_tests:
config.available_features.add("mlir_arm_sve_tests")
More information about the Mlir-commits
mailing list