[Mlir-commits] [mlir] [mlir][spirv] Fix int type declaration duplication when serializing (PR #143108)

[Davide Grohmann]

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/2] [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/2] 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
+  }