Sort functions inside the serializer.

Author: IgWod-IMG

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)

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()) {
+    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()) {

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

@@ -1,34 +1,14 @@
 // 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
         %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}}
+    // 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",

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

@@ -6,20 +6,20 @@
 // 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<
-        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
     }
+    // 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 @inside.func() -> () "Pure" attributes {} {spirv.Return}
 }