Skip to content

Commit ba85d0d

Browse files
IgWod-IMGIgWod-IMG
committed
[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.
1 parent d130f40 commit ba85d0d

File tree

3 files changed

+57
-18
lines changed

3 files changed

+57
-18
lines changed

‎mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1516,6 +1516,7 @@ LogicalResult spirv::ModuleOp::verifyRegions() {
15161516
DenseMap<std::pair<spirv::FuncOp, spirv::ExecutionModel>, spirv::EntryPointOp>
15171517
entryPoints;
15181518
mlir::SymbolTable table(*this);
1519+
bool encounteredFuncDefinition = false;
15191520

15201521
for (auto &op : *getBody()) {
15211522
if (op.getDialect() != dialect)
@@ -1561,10 +1562,23 @@ LogicalResult spirv::ModuleOp::verifyRegions() {
15611562
auto hasImportLinkage =
15621563
linkageAttr && (linkageAttr.value().getLinkageType().getValue() ==
15631564
spirv::LinkageType::Import);
1564-
if (funcOp.isExternal() && !hasImportLinkage)
1565-
return op.emitError(
1566-
"'spirv.module' cannot contain external functions "
1567-
"without 'Import' linkage_attributes (LinkageAttributes)");
1565+
if (funcOp.isExternal()) {
1566+
if (!hasImportLinkage)
1567+
return op.emitError(
1568+
"'spirv.module' cannot contain external functions "
1569+
"without 'Import' linkage_attributes (LinkageAttributes)");
1570+
// This is stronger than necessary. It should be sufficient to
1571+
// ensure any declarations precede their uses and not all definitions,
1572+
// however this allows to avoid analysing every function in the module
1573+
// this way.
1574+
if (encounteredFuncDefinition)
1575+
return op.emitError("all functions declarations in 'spirv.module' "
1576+
"must happen before any definitions");
1577+
}
1578+
1579+
// In SPIR-V "declarations" are functions without a body and "definitions"
1580+
// functions with a body.
1581+
encounteredFuncDefinition |= !funcOp.getBody().empty();
15681582

15691583
// TODO: move this check to spirv.func.
15701584
for (auto &block : funcOp)

‎mlir/test/Dialect/SPIRV/IR/function-decorations.mlir‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,34 @@
11
// RUN: mlir-opt -split-input-file -verify-diagnostics %s | FileCheck %s
22

33
spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage], []> {
4+
// CHECK: linkage_attributes = #spirv.linkage_attributes<linkage_name = "outside.func", linkage_type = <Import>>
5+
spirv.func @outside.func.with.linkage(%arg0 : i8) -> () "Pure" attributes {
6+
linkage_attributes=#spirv.linkage_attributes<
7+
linkage_name="outside.func",
8+
linkage_type=<Import>
9+
>
10+
}
411
spirv.func @linkage_attr_test_kernel() "DontInline" attributes {} {
512
%uchar_0 = spirv.Constant 0 : i8
613
%ushort_1 = spirv.Constant 1 : i16
714
%uint_0 = spirv.Constant 0 : i32
815
spirv.FunctionCall @outside.func.with.linkage(%uchar_0):(i8) -> ()
916
spirv.Return
1017
}
11-
// CHECK: linkage_attributes = #spirv.linkage_attributes<linkage_name = "outside.func", linkage_type = <Import>>
18+
spirv.func @inside.func() -> () "Pure" attributes {} {spirv.Return}
19+
}
20+
21+
// -----
22+
23+
spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage, Int8, Int16], []> {
24+
spirv.func @linkage_attr_test_kernel() "DontInline" attributes {} {
25+
%uchar_0 = spirv.Constant 0 : i8
26+
%ushort_1 = spirv.Constant 1 : i16
27+
%uint_0 = spirv.Constant 0 : i32
28+
spirv.FunctionCall @outside.func.with.linkage(%uchar_0):(i8) -> ()
29+
spirv.Return
30+
}
31+
// expected-error@+1 {{all functions declarations in 'spirv.module' must happen before any definitions}}
1232
spirv.func @outside.func.with.linkage(%arg0 : i8) -> () "Pure" attributes {
1333
linkage_attributes=#spirv.linkage_attributes<
1434
linkage_name="outside.func",

‎mlir/test/Target/SPIRV/function-decorations.mlir‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,32 @@
11
// RUN: mlir-translate --no-implicit-module --test-spirv-roundtrip --split-input-file %s | FileCheck %s
22

3-
spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage], []> {
4-
spirv.func @linkage_attr_test_kernel() "DontInline" attributes {} {
5-
%uchar_0 = spirv.Constant 0 : i8
6-
%ushort_1 = spirv.Constant 1 : i16
7-
%uint_0 = spirv.Constant 0 : i32
8-
spirv.FunctionCall @outside.func.with.linkage(%uchar_0):(i8) -> ()
9-
spirv.Return
10-
}
3+
// RUN: %if spirv-tools %{ rm -rf %t %}
4+
// RUN: %if spirv-tools %{ mkdir %t %}
5+
// RUN: %if spirv-tools %{ mlir-translate --no-implicit-module --serialize-spirv --split-input-file --spirv-save-validation-files-with-prefix=%t/module %s %}
6+
// RUN: %if spirv-tools %{ spirv-val %t %}
7+
8+
spirv.module Logical GLSL450 requires #spirv.vce<v1.0, [Shader, Linkage, Int8, Int16], []> {
119
// CHECK: linkage_attributes = #spirv.linkage_attributes<linkage_name = "outside.func", linkage_type = <Import>>
1210
spirv.func @outside.func.with.linkage(%arg0 : i8) -> () "Pure" attributes {
1311
linkage_attributes=#spirv.linkage_attributes<
1412
linkage_name="outside.func",
1513
linkage_type=<Import>
1614
>
1715
}
16+
spirv.func @linkage_attr_test_kernel() "DontInline" attributes {} {
17+
%uchar_0 = spirv.Constant 0 : i8
18+
%ushort_1 = spirv.Constant 1 : i16
19+
%uint_0 = spirv.Constant 0 : i32
20+
spirv.FunctionCall @outside.func.with.linkage(%uchar_0):(i8) -> ()
21+
spirv.Return
22+
}
1823
spirv.func @inside.func() -> () "Pure" attributes {} {spirv.Return}
1924
}
2025

2126
// -----
2227

2328
spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
24-
[Shader, PhysicalStorageBufferAddresses], [SPV_KHR_physical_storage_buffer]> {
29+
[Shader, PhysicalStorageBufferAddresses, Linkage], [SPV_KHR_physical_storage_buffer]> {
2530
// CHECK-LABEL: spirv.func @func_arg_decoration_aliased(%{{.*}}: !spirv.ptr<i32, PhysicalStorageBuffer> {spirv.decoration = #spirv.decoration<Aliased>})
2631
spirv.func @func_arg_decoration_aliased(
2732
%arg0 : !spirv.ptr<i32, PhysicalStorageBuffer> { spirv.decoration = #spirv.decoration<Aliased> }
@@ -33,7 +38,7 @@ spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
3338
// -----
3439

3540
spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
36-
[Shader, PhysicalStorageBufferAddresses], [SPV_KHR_physical_storage_buffer]> {
41+
[Shader, PhysicalStorageBufferAddresses, Linkage], [SPV_KHR_physical_storage_buffer]> {
3742
// CHECK-LABEL: spirv.func @func_arg_decoration_restrict(%{{.*}}: !spirv.ptr<i32, PhysicalStorageBuffer> {spirv.decoration = #spirv.decoration<Restrict>})
3843
spirv.func @func_arg_decoration_restrict(
3944
%arg0 : !spirv.ptr<i32,PhysicalStorageBuffer> { spirv.decoration = #spirv.decoration<Restrict> }
@@ -45,7 +50,7 @@ spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
4550
// -----
4651

4752
spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
48-
[Shader, PhysicalStorageBufferAddresses], [SPV_KHR_physical_storage_buffer]> {
53+
[Shader, PhysicalStorageBufferAddresses, Linkage, GenericPointer], [SPV_KHR_physical_storage_buffer]> {
4954
// CHECK-LABEL: spirv.func @func_arg_decoration_aliased_pointer(%{{.*}}: !spirv.ptr<!spirv.ptr<i32, PhysicalStorageBuffer>, Generic> {spirv.decoration = #spirv.decoration<AliasedPointer>})
5055
spirv.func @func_arg_decoration_aliased_pointer(
5156
%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,
5762
// -----
5863

5964
spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
60-
[Shader, PhysicalStorageBufferAddresses], [SPV_KHR_physical_storage_buffer]> {
65+
[Shader, PhysicalStorageBufferAddresses, Linkage, GenericPointer], [SPV_KHR_physical_storage_buffer]> {
6166
// CHECK-LABEL: spirv.func @func_arg_decoration_restrict_pointer(%{{.*}}: !spirv.ptr<!spirv.ptr<i32, PhysicalStorageBuffer>, Generic> {spirv.decoration = #spirv.decoration<RestrictPointer>})
6267
spirv.func @func_arg_decoration_restrict_pointer(
6368
%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,
6974
// -----
7075

7176
spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.0,
72-
[Shader, PhysicalStorageBufferAddresses], [SPV_KHR_physical_storage_buffer]> {
77+
[Shader, PhysicalStorageBufferAddresses, Linkage], [SPV_KHR_physical_storage_buffer]> {
7378
// CHECK-LABEL: spirv.func @fn1(%{{.*}}: i32, %{{.*}}: !spirv.ptr<i32, PhysicalStorageBuffer> {spirv.decoration = #spirv.decoration<Aliased>})
7479
spirv.func @fn1(
7580
%arg0: i32,

0 commit comments

Comments
 (0)