Skip to content

[mlir][spirv] Fix int type declaration duplication when serializing #143108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 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 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);
}

typeID = getTypeID(type);
if (typeID)
return success();
Expand Down
6 changes: 6 additions & 0 deletions mlir/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
5 changes: 4 additions & 1 deletion mlir/test/Target/SPIRV/constant.mlir
Original file line number Diff line number Diff line change
@@ -1,6 +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], []> {
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
Expand Down Expand Up @@ -305,4 +306,6 @@ spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader], []> {
%coop = spirv.Constant dense<4> : !spirv.coopmatrix<16x16xi8, Subgroup, MatrixAcc>
spirv.ReturnValue %coop : !spirv.coopmatrix<16x16xi8, Subgroup, MatrixAcc>
}

spirv.EntryPoint "GLCompute" @bool_const
}
1 change: 1 addition & 0 deletions mlir/test/lit.cfg.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 7 additions & 0 deletions mlir/test/lit.local.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
if not "SPIRV" in config.root.targets:
config.unsupported = True
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the SPIRV backend to run MLIR unit-tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see this comment: #143108 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this -- if this disables all mlir tests, we should revert the PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was partially fixed in #144685 but now disables all spirv tests...
@davidegrohmann can you prepare a fix by EoD? Otherwise we should revert this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert PR: #144773

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I miss your comment it was already EoD for me. Thanks for reverting the patch. I will prepare a new PR without the problematic code.


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")))
4 changes: 3 additions & 1 deletion mlir/test/lit.site.cfg.py.in
Original file line number Diff line number Diff line change
Expand Up @@ -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@"
Expand Down Expand Up @@ -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")
Expand Down
Loading