From ba85d0d15b706043b7ce4bc624b5a47aac04f552 Mon Sep 17 00:00:00 2001 From: Igor Wodiany 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, 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 { + // CHECK: linkage_attributes = #spirv.linkage_attributes> + spirv.func @outside.func.with.linkage(%arg0 : i8) -> () "Pure" attributes { + linkage_attributes=#spirv.linkage_attributes< + linkage_name="outside.func", + linkage_type= + > + } 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 { spirv.FunctionCall @outside.func.with.linkage(%uchar_0):(i8) -> () spirv.Return } - // CHECK: linkage_attributes = #spirv.linkage_attributes> + spirv.func @inside.func() -> () "Pure" attributes {} {spirv.Return} +} + +// ----- + +spirv.module Logical GLSL450 requires #spirv.vce { + 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@+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 { - 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 { // CHECK: linkage_attributes = #spirv.linkage_attributes> 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 { linkage_type= > } + 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 { + [Shader, PhysicalStorageBufferAddresses, Linkage], [SPV_KHR_physical_storage_buffer]> { // CHECK-LABEL: spirv.func @func_arg_decoration_aliased(%{{.*}}: !spirv.ptr {spirv.decoration = #spirv.decoration}) spirv.func @func_arg_decoration_aliased( %arg0 : !spirv.ptr { spirv.decoration = #spirv.decoration } @@ -33,7 +38,7 @@ spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce { + [Shader, PhysicalStorageBufferAddresses, Linkage], [SPV_KHR_physical_storage_buffer]> { // CHECK-LABEL: spirv.func @func_arg_decoration_restrict(%{{.*}}: !spirv.ptr {spirv.decoration = #spirv.decoration}) spirv.func @func_arg_decoration_restrict( %arg0 : !spirv.ptr { spirv.decoration = #spirv.decoration } @@ -45,7 +50,7 @@ spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce { + [Shader, PhysicalStorageBufferAddresses, Linkage, GenericPointer], [SPV_KHR_physical_storage_buffer]> { // CHECK-LABEL: spirv.func @func_arg_decoration_aliased_pointer(%{{.*}}: !spirv.ptr, Generic> {spirv.decoration = #spirv.decoration}) spirv.func @func_arg_decoration_aliased_pointer( %arg0 : !spirv.ptr, Generic> { spirv.decoration = #spirv.decoration } @@ -57,7 +62,7 @@ spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce { + [Shader, PhysicalStorageBufferAddresses, Linkage, GenericPointer], [SPV_KHR_physical_storage_buffer]> { // CHECK-LABEL: spirv.func @func_arg_decoration_restrict_pointer(%{{.*}}: !spirv.ptr, Generic> {spirv.decoration = #spirv.decoration}) spirv.func @func_arg_decoration_restrict_pointer( %arg0 : !spirv.ptr, Generic> { spirv.decoration = #spirv.decoration } @@ -69,7 +74,7 @@ spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce { + [Shader, PhysicalStorageBufferAddresses, Linkage], [SPV_KHR_physical_storage_buffer]> { // CHECK-LABEL: spirv.func @fn1(%{{.*}}: i32, %{{.*}}: !spirv.ptr {spirv.decoration = #spirv.decoration}) spirv.func @fn1( %arg0: i32, From 9b221af32566c5ee5576522378bd1b99484f152d Mon Sep 17 00:00:00 2001 From: Igor Wodiany 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, 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(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 { - // CHECK: linkage_attributes = #spirv.linkage_attributes> - spirv.func @outside.func.with.linkage(%arg0 : i8) -> () "Pure" attributes { - linkage_attributes=#spirv.linkage_attributes< - linkage_name="outside.func", - linkage_type= - > - } - 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 { 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 () spirv.Return } - // expected-error@+1 {{all functions declarations in 'spirv.module' must happen before any definitions}} + // CHECK: linkage_attributes = #spirv.linkage_attributes> 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 { + // CHECK: spirv.func @outside.func.with.linkage(i8) "Pure" attributes // CHECK: linkage_attributes = #spirv.linkage_attributes> - spirv.func @outside.func.with.linkage(%arg0 : i8) -> () "Pure" attributes { - linkage_attributes=#spirv.linkage_attributes< - linkage_name="outside.func", - linkage_type= - > - } + // 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 () spirv.Return } + spirv.func @outside.func.with.linkage(%arg0 : i8) -> () "Pure" attributes { + linkage_attributes=#spirv.linkage_attributes< + linkage_name="outside.func", + linkage_type= + > + } spirv.func @inside.func() -> () "Pure" attributes {} {spirv.Return} }