[Mlir-commits] [mlir] [mlir][spirv] Ensure function declarations precede definitions (PR #164956)
Igor Wodiany
llvmlistbot at llvm.org
Fri Oct 24 08:14:01 PDT 2025
https://github.com/IgWod-IMG updated https://github.com/llvm/llvm-project/pull/164956
>From ba85d0d15b706043b7ce4bc624b5a47aac04f552 Mon Sep 17 00:00:00 2001
From: Igor Wodiany <igor.wodiany at imgtec.com>
Date: Thu, 23 Oct 2025 17:34:41 +0100
Subject: [PATCH 1/2] [mlir][spirv] Ensure function declarations precede
definitions
SPIR-V spec requires that any calls to external functions are preceded
by declarations of those external functions. To simplify the verification
we strengthen the condition so any external declarations must precede
any function definitions - this ensures that external functions are always
declared before being used. As an alternative we could allow arbitrary ordering
in MLIR and sort functions in the serialization, but in this case it
feels like aligning MLIR representation with SPIR-V is a cleaner approach.
---
mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp | 22 ++++++++++---
.../SPIRV/IR/function-decorations.mlir | 22 ++++++++++++-
.../Target/SPIRV/function-decorations.mlir | 31 +++++++++++--------
3 files changed, 57 insertions(+), 18 deletions(-)
diff --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
index fe50865bb7c49..6c96ef371d7a4 100644
--- a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
@@ -1516,6 +1516,7 @@ LogicalResult spirv::ModuleOp::verifyRegions() {
DenseMap<std::pair<spirv::FuncOp, spirv::ExecutionModel>, spirv::EntryPointOp>
entryPoints;
mlir::SymbolTable table(*this);
+ bool encounteredFuncDefinition = false;
for (auto &op : *getBody()) {
if (op.getDialect() != dialect)
@@ -1561,10 +1562,23 @@ LogicalResult spirv::ModuleOp::verifyRegions() {
auto hasImportLinkage =
linkageAttr && (linkageAttr.value().getLinkageType().getValue() ==
spirv::LinkageType::Import);
- if (funcOp.isExternal() && !hasImportLinkage)
- return op.emitError(
- "'spirv.module' cannot contain external functions "
- "without 'Import' linkage_attributes (LinkageAttributes)");
+ if (funcOp.isExternal()) {
+ if (!hasImportLinkage)
+ return op.emitError(
+ "'spirv.module' cannot contain external functions "
+ "without 'Import' linkage_attributes (LinkageAttributes)");
+ // This is stronger than necessary. It should be sufficient to
+ // ensure any declarations precede their uses and not all definitions,
+ // however this allows to avoid analysing every function in the module
+ // this way.
+ if (encounteredFuncDefinition)
+ return op.emitError("all functions declarations in 'spirv.module' "
+ "must happen before any definitions");
+ }
+
+ // In SPIR-V "declarations" are functions without a body and "definitions"
+ // functions with a body.
+ encounteredFuncDefinition |= !funcOp.getBody().empty();
// TODO: move this check to spirv.func.
for (auto &block : funcOp)
diff --git a/mlir/test/Dialect/SPIRV/IR/function-decorations.mlir b/mlir/test/Dialect/SPIRV/IR/function-decorations.mlir
index f09767a416f6b..a83cbf0fc0919 100644
--- a/mlir/test/Dialect/SPIRV/IR/function-decorations.mlir
+++ b/mlir/test/Dialect/SPIRV/IR/function-decorations.mlir
@@ -1,6 +1,13 @@
// RUN: mlir-opt -split-input-file -verify-diagnostics %s | FileCheck %s
spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage], []> {
+ // 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 @linkage_attr_test_kernel() "DontInline" attributes {} {
%uchar_0 = spirv.Constant 0 : i8
%ushort_1 = spirv.Constant 1 : i16
@@ -8,7 +15,20 @@ spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage], []> {
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 @inside.func() -> () "Pure" attributes {} {spirv.Return}
+}
+
+// -----
+
+spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage, Int8, Int16], []> {
+ 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
+ }
+ // expected-error at +1 {{all functions declarations in 'spirv.module' must happen before any definitions}}
spirv.func @outside.func.with.linkage(%arg0 : i8) -> () "Pure" attributes {
linkage_attributes=#spirv.linkage_attributes<
linkage_name="outside.func",
diff --git a/mlir/test/Target/SPIRV/function-decorations.mlir b/mlir/test/Target/SPIRV/function-decorations.mlir
index cf6edaa0a3d5b..b6f4d08271fa4 100644
--- a/mlir/test/Target/SPIRV/function-decorations.mlir
+++ b/mlir/test/Target/SPIRV/function-decorations.mlir
@@ -1,13 +1,11 @@
// RUN: mlir-translate --no-implicit-module --test-spirv-roundtrip --split-input-file %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
- }
+// RUN: %if spirv-tools %{ rm -rf %t %}
+// RUN: %if spirv-tools %{ mkdir %t %}
+// RUN: %if spirv-tools %{ mlir-translate --no-implicit-module --serialize-spirv --split-input-file --spirv-save-validation-files-with-prefix=%t/module %s %}
+// RUN: %if spirv-tools %{ spirv-val %t %}
+
+spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage, Int8, Int16], []> {
// 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<
@@ -15,13 +13,20 @@ spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage], []> {
linkage_type=<Import>
>
}
+ 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
+ }
spirv.func @inside.func() -> () "Pure" attributes {} {spirv.Return}
}
// -----
spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
- [Shader, PhysicalStorageBufferAddresses], [SPV_KHR_physical_storage_buffer]> {
+ [Shader, PhysicalStorageBufferAddresses, Linkage], [SPV_KHR_physical_storage_buffer]> {
// CHECK-LABEL: spirv.func @func_arg_decoration_aliased(%{{.*}}: !spirv.ptr<i32, PhysicalStorageBuffer> {spirv.decoration = #spirv.decoration<Aliased>})
spirv.func @func_arg_decoration_aliased(
%arg0 : !spirv.ptr<i32, PhysicalStorageBuffer> { spirv.decoration = #spirv.decoration<Aliased> }
@@ -33,7 +38,7 @@ spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
// -----
spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
- [Shader, PhysicalStorageBufferAddresses], [SPV_KHR_physical_storage_buffer]> {
+ [Shader, PhysicalStorageBufferAddresses, Linkage], [SPV_KHR_physical_storage_buffer]> {
// CHECK-LABEL: spirv.func @func_arg_decoration_restrict(%{{.*}}: !spirv.ptr<i32, PhysicalStorageBuffer> {spirv.decoration = #spirv.decoration<Restrict>})
spirv.func @func_arg_decoration_restrict(
%arg0 : !spirv.ptr<i32,PhysicalStorageBuffer> { spirv.decoration = #spirv.decoration<Restrict> }
@@ -45,7 +50,7 @@ spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
// -----
spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
- [Shader, PhysicalStorageBufferAddresses], [SPV_KHR_physical_storage_buffer]> {
+ [Shader, PhysicalStorageBufferAddresses, Linkage, GenericPointer], [SPV_KHR_physical_storage_buffer]> {
// CHECK-LABEL: spirv.func @func_arg_decoration_aliased_pointer(%{{.*}}: !spirv.ptr<!spirv.ptr<i32, PhysicalStorageBuffer>, Generic> {spirv.decoration = #spirv.decoration<AliasedPointer>})
spirv.func @func_arg_decoration_aliased_pointer(
%arg0 : !spirv.ptr<!spirv.ptr<i32,PhysicalStorageBuffer>, Generic> { spirv.decoration = #spirv.decoration<AliasedPointer> }
@@ -57,7 +62,7 @@ spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
// -----
spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
- [Shader, PhysicalStorageBufferAddresses], [SPV_KHR_physical_storage_buffer]> {
+ [Shader, PhysicalStorageBufferAddresses, Linkage, GenericPointer], [SPV_KHR_physical_storage_buffer]> {
// CHECK-LABEL: spirv.func @func_arg_decoration_restrict_pointer(%{{.*}}: !spirv.ptr<!spirv.ptr<i32, PhysicalStorageBuffer>, Generic> {spirv.decoration = #spirv.decoration<RestrictPointer>})
spirv.func @func_arg_decoration_restrict_pointer(
%arg0 : !spirv.ptr<!spirv.ptr<i32,PhysicalStorageBuffer>, Generic> { spirv.decoration = #spirv.decoration<RestrictPointer> }
@@ -69,7 +74,7 @@ spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
// -----
spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
- [Shader, PhysicalStorageBufferAddresses], [SPV_KHR_physical_storage_buffer]> {
+ [Shader, PhysicalStorageBufferAddresses, Linkage], [SPV_KHR_physical_storage_buffer]> {
// CHECK-LABEL: spirv.func @fn1(%{{.*}}: i32, %{{.*}}: !spirv.ptr<i32, PhysicalStorageBuffer> {spirv.decoration = #spirv.decoration<Aliased>})
spirv.func @fn1(
%arg0: i32,
>From 9b221af32566c5ee5576522378bd1b99484f152d Mon Sep 17 00:00:00 2001
From: Igor Wodiany <igor.wodiany at imgtec.com>
Date: Fri, 24 Oct 2025 15:30:28 +0100
Subject: [PATCH 2/2] Sort functions inside the serializer.
---
mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp | 22 ++++---------------
.../Target/SPIRV/Serialization/Serializer.cpp | 18 +++++++++++++++
.../SPIRV/IR/function-decorations.mlir | 22 +------------------
.../Target/SPIRV/function-decorations.mlir | 15 ++++++++-----
4 files changed, 32 insertions(+), 45 deletions(-)
diff --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
index 6c96ef371d7a4..fe50865bb7c49 100644
--- a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
@@ -1516,7 +1516,6 @@ LogicalResult spirv::ModuleOp::verifyRegions() {
DenseMap<std::pair<spirv::FuncOp, spirv::ExecutionModel>, spirv::EntryPointOp>
entryPoints;
mlir::SymbolTable table(*this);
- bool encounteredFuncDefinition = false;
for (auto &op : *getBody()) {
if (op.getDialect() != dialect)
@@ -1562,23 +1561,10 @@ LogicalResult spirv::ModuleOp::verifyRegions() {
auto hasImportLinkage =
linkageAttr && (linkageAttr.value().getLinkageType().getValue() ==
spirv::LinkageType::Import);
- if (funcOp.isExternal()) {
- if (!hasImportLinkage)
- return op.emitError(
- "'spirv.module' cannot contain external functions "
- "without 'Import' linkage_attributes (LinkageAttributes)");
- // This is stronger than necessary. It should be sufficient to
- // ensure any declarations precede their uses and not all definitions,
- // however this allows to avoid analysing every function in the module
- // this way.
- if (encounteredFuncDefinition)
- return op.emitError("all functions declarations in 'spirv.module' "
- "must happen before any definitions");
- }
-
- // In SPIR-V "declarations" are functions without a body and "definitions"
- // functions with a body.
- encounteredFuncDefinition |= !funcOp.getBody().empty();
+ if (funcOp.isExternal() && !hasImportLinkage)
+ return op.emitError(
+ "'spirv.module' cannot contain external functions "
+ "without 'Import' linkage_attributes (LinkageAttributes)");
// TODO: move this check to spirv.func.
for (auto &block : funcOp)
diff --git a/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp b/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
index b56e7788625f5..eb9040dbcd03c 100644
--- a/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
+++ b/mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
@@ -89,6 +89,22 @@ static bool isZeroValue(Attribute attr) {
return false;
}
+/// Move all functions declaration before functions definitions. In SPIR-V
+/// "declarations" are functions without a body and "definitions" functions
+/// with a body. This is stronger than necessary. It should be sufficient to
+/// ensure any declarations precede their uses and not all definitions, however
+/// this allows to avoid analysing every function in the module this way.
+static void moveFuncDeclarationsToTop(spirv::ModuleOp moduleOp) {
+ Block::OpListType &ops = moduleOp.getBody()->getOperations();
+ if (ops.empty())
+ return;
+ Operation &firstOp = ops.front();
+ for (Operation &op : llvm::drop_begin(ops))
+ if (auto funcOp = dyn_cast<spirv::FuncOp>(op))
+ if (funcOp.getBody().empty())
+ funcOp->moveBefore(&firstOp);
+}
+
namespace mlir {
namespace spirv {
@@ -119,6 +135,8 @@ LogicalResult Serializer::serialize() {
processMemoryModel();
processDebugInfo();
+ moveFuncDeclarationsToTop(module);
+
// Iterate over the module body to serialize it. Assumptions are that there is
// only one basic block in the moduleOp
for (auto &op : *module.getBody()) {
diff --git a/mlir/test/Dialect/SPIRV/IR/function-decorations.mlir b/mlir/test/Dialect/SPIRV/IR/function-decorations.mlir
index a83cbf0fc0919..f09767a416f6b 100644
--- a/mlir/test/Dialect/SPIRV/IR/function-decorations.mlir
+++ b/mlir/test/Dialect/SPIRV/IR/function-decorations.mlir
@@ -1,26 +1,6 @@
// RUN: mlir-opt -split-input-file -verify-diagnostics %s | FileCheck %s
spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage], []> {
- // 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 @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
- }
- spirv.func @inside.func() -> () "Pure" attributes {} {spirv.Return}
-}
-
-// -----
-
-spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage, Int8, Int16], []> {
spirv.func @linkage_attr_test_kernel() "DontInline" attributes {} {
%uchar_0 = spirv.Constant 0 : i8
%ushort_1 = spirv.Constant 1 : i16
@@ -28,7 +8,7 @@ spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage, Int8, I
spirv.FunctionCall @outside.func.with.linkage(%uchar_0):(i8) -> ()
spirv.Return
}
- // expected-error at +1 {{all functions declarations in 'spirv.module' must happen before any definitions}}
+ // 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",
diff --git a/mlir/test/Target/SPIRV/function-decorations.mlir b/mlir/test/Target/SPIRV/function-decorations.mlir
index b6f4d08271fa4..a47b39b0b9339 100644
--- a/mlir/test/Target/SPIRV/function-decorations.mlir
+++ b/mlir/test/Target/SPIRV/function-decorations.mlir
@@ -6,13 +6,10 @@
// RUN: %if spirv-tools %{ spirv-val %t %}
spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage, Int8, Int16], []> {
+ // CHECK: spirv.func @outside.func.with.linkage(i8) "Pure" attributes
// 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>
- >
- }
+ // CHECK: spirv.func @linkage_attr_test_kernel() "DontInline" {
+ // CHECK: spirv.func @inside.func() "Pure" {
spirv.func @linkage_attr_test_kernel() "DontInline" attributes {} {
%uchar_0 = spirv.Constant 0 : i8
%ushort_1 = spirv.Constant 1 : i16
@@ -20,6 +17,12 @@ spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage, Int8, I
spirv.FunctionCall @outside.func.with.linkage(%uchar_0):(i8) -> ()
spirv.Return
}
+ 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}
}
More information about the Mlir-commits
mailing list