From fdfac9146abe9590745f3f7d6cc572899e19cf90 Mon Sep 17 00:00:00 2001 From: Davide Grohmann 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 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 &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 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 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 &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 { +spirv.module Logical GLSL450 requires #spirv.vce { // CHECK-LABEL: @bool_const spirv.func @bool_const() -> () "None" { // CHECK: spirv.Constant true @@ -277,4 +277,6 @@ spirv.module Logical GLSL450 requires #spirv.vce { %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 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 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 { // CHECK-LABEL: @bool_const From afab0c51bac123d3b47ff0be5be45abc102a0a97 Mon Sep 17 00:00:00 2001 From: Davide Grohmann 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 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 { +spirv.module Logical Vulkan requires #spirv.vce { // 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 -# requires to be in the set of available features. +# requires 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")