Skip to content

Conversation

@IgWod-IMG
Copy link
Contributor

@IgWod-IMG IgWod-IMG commented Oct 24, 2025

SPIR-V spec requires that any calls to external functions are preceded by declarations of those external functions. To simplify the implementation, we sort functions in the serializer using a stronger condition: any functions declarations are moved before any functions definitions - this ensures that external functions are always declared before being used.

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.
@llvmbot
Copy link
Member

llvmbot commented Oct 24, 2025

@llvm/pr-subscribers-mlir

Author: Igor Wodiany (IgWod-IMG)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/164956.diff

3 Files Affected:

  • (modified) mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp (+18-4)
  • (modified) mlir/test/Dialect/SPIRV/IR/function-decorations.mlir (+21-1)
  • (modified) mlir/test/Target/SPIRV/function-decorations.mlir (+18-13)
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@+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,

@llvmbot
Copy link
Member

llvmbot commented Oct 24, 2025

@llvm/pr-subscribers-mlir-spirv

Author: Igor Wodiany (IgWod-IMG)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/164956.diff

3 Files Affected:

  • (modified) mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp (+18-4)
  • (modified) mlir/test/Dialect/SPIRV/IR/function-decorations.mlir (+21-1)
  • (modified) mlir/test/Target/SPIRV/function-decorations.mlir (+18-13)
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@+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,

Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

As an alternative we could allow arbitrary ordering in MLIR and sort functions in the serialization

I'd prefer this since mlir doesn't have the same limitations

@IgWod-IMG
Copy link
Contributor Author

I'd prefer this since mlir doesn't have the same limitations

This was my initial implementation then I convinced myself to follow the other approach. I have now added "sorting" inside the serializer. I have also updated the PR description to reflect it.

Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

Should we have a roundtrip test that shows that function decls were moved to the top?

/// 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()) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: flip this condition and return early instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, it was a leftover from before I moved code into a function

@IgWod-IMG IgWod-IMG force-pushed the img_fix-serializer-func-decl branch from 3159c93 to 9b221af Compare October 24, 2025 15:13
@IgWod-IMG
Copy link
Contributor Author

Should we have a roundtrip test that shows that function decls were moved to the top?

I have updated the existing test, so it checks the order of functions after roundtrip. Let me know if we need any extra tests.

@IgWod-IMG IgWod-IMG merged commit 046ed90 into llvm:main Oct 27, 2025
11 checks passed
@IgWod-IMG IgWod-IMG deleted the img_fix-serializer-func-decl branch October 27, 2025 09:28
dvbuka pushed a commit to dvbuka/llvm-project that referenced this pull request Oct 27, 2025
…164956)

SPIR-V spec requires that any calls to external functions are preceded
by declarations of those external functions. To simplify the
implementation, we sort functions in the serializer using a stronger
condition: any functions declarations are moved before any functions
definitions - this ensures that external functions are always declared
before being used.
Lukacma pushed a commit to Lukacma/llvm-project that referenced this pull request Oct 29, 2025
…164956)

SPIR-V spec requires that any calls to external functions are preceded
by declarations of those external functions. To simplify the
implementation, we sort functions in the serializer using a stronger
condition: any functions declarations are moved before any functions
definitions - this ensures that external functions are always declared
before being used.
aokblast pushed a commit to aokblast/llvm-project that referenced this pull request Oct 30, 2025
…164956)

SPIR-V spec requires that any calls to external functions are preceded
by declarations of those external functions. To simplify the
implementation, we sort functions in the serializer using a stronger
condition: any functions declarations are moved before any functions
definitions - this ensures that external functions are always declared
before being used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants