[Mlir-commits] [mlir] [mlir][spirv] Remove nonlocal call verification. (PR #159399)
Erick Ochoa Lopez
llvmlistbot at llvm.org
Thu Sep 18 06:29:02 PDT 2025
https://github.com/amd-eochoalo updated https://github.com/llvm/llvm-project/pull/159399
>From aed3836d1e30fc59c2da968541a4c58bbfcaed68 Mon Sep 17 00:00:00 2001
From: Erick Ochoa <erick.ochoalopez at amd.com>
Date: Wed, 17 Sep 2025 12:47:33 -0400
Subject: [PATCH 1/6] [mlir][spirv] Remove nonlocal call verification.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
`spirv.FunctionCall`'s verifier was being too aggressive.
It included verification of non-local properties by
looking at the callee's definition.
This caused problems in cases where callee had verification
errors and could lead to null pointer dereferencing.
According to MLIR's developers guide
TLDR: only verify local aspects of an operation,
in particular don’t follow def-use chains
(don’t look at the producer of any operand or the user
of any results).
https://mlir.llvm.org/getting_started/DeveloperGuide/#ir-verifier
---
mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp | 30 --------------------
1 file changed, 30 deletions(-)
diff --git a/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp b/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp
index 890406df74e72..95d63ddee6824 100644
--- a/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp
@@ -160,42 +160,12 @@ LogicalResult FunctionCallOp::verify() {
<< fnName.getValue() << "' not found in nearest symbol table";
}
- auto functionType = funcOp.getFunctionType();
-
if (getNumResults() > 1) {
return emitOpError(
"expected callee function to have 0 or 1 result, but provided ")
<< getNumResults();
}
- if (functionType.getNumInputs() != getNumOperands()) {
- return emitOpError("has incorrect number of operands for callee: expected ")
- << functionType.getNumInputs() << ", but provided "
- << getNumOperands();
- }
-
- for (uint32_t i = 0, e = functionType.getNumInputs(); i != e; ++i) {
- if (getOperand(i).getType() != functionType.getInput(i)) {
- return emitOpError("operand type mismatch: expected operand type ")
- << functionType.getInput(i) << ", but provided "
- << getOperand(i).getType() << " for operand number " << i;
- }
- }
-
- if (functionType.getNumResults() != getNumResults()) {
- return emitOpError(
- "has incorrect number of results has for callee: expected ")
- << functionType.getNumResults() << ", but provided "
- << getNumResults();
- }
-
- if (getNumResults() &&
- (getResult(0).getType() != functionType.getResult(0))) {
- return emitOpError("result type mismatch: expected ")
- << functionType.getResult(0) << ", but provided "
- << getResult(0).getType();
- }
-
return success();
}
>From bc34970cf59fe36b3e311b3f12985c62d75b5f03 Mon Sep 17 00:00:00 2001
From: Erick Ochoa <erick.ochoalopez at amd.com>
Date: Wed, 17 Sep 2025 12:56:57 -0400
Subject: [PATCH 2/6] Update tests
---
.../Dialect/SPIRV/IR/control-flow-ops.mlir | 42 -------------------
1 file changed, 42 deletions(-)
diff --git a/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir b/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir
index 8ec0bf5bbaacf..f29f673b3dd3a 100644
--- a/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir
+++ b/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir
@@ -210,48 +210,6 @@ spirv.module Logical GLSL450 {
// -----
-spirv.module Logical GLSL450 {
- spirv.func @f_result_type_mismatch(%arg0 : i32, %arg1 : i32) -> () "None" {
- // expected-error @+1 {{has incorrect number of results has for callee: expected 0, but provided 1}}
- %1 = spirv.FunctionCall @f_result_type_mismatch(%arg0, %arg0) : (i32, i32) -> (i32)
- spirv.Return
- }
-}
-
-// -----
-
-spirv.module Logical GLSL450 {
- spirv.func @f_type_mismatch(%arg0 : i32, %arg1 : i32) -> () "None" {
- // expected-error @+1 {{has incorrect number of operands for callee: expected 2, but provided 1}}
- spirv.FunctionCall @f_type_mismatch(%arg0) : (i32) -> ()
- spirv.Return
- }
-}
-
-// -----
-
-spirv.module Logical GLSL450 {
- spirv.func @f_type_mismatch(%arg0 : i32, %arg1 : i32) -> () "None" {
- %0 = spirv.Constant 2.0 : f32
- // expected-error @+1 {{operand type mismatch: expected operand type 'i32', but provided 'f32' for operand number 1}}
- spirv.FunctionCall @f_type_mismatch(%arg0, %0) : (i32, f32) -> ()
- spirv.Return
- }
-}
-
-// -----
-
-spirv.module Logical GLSL450 {
- spirv.func @f_type_mismatch(%arg0 : i32, %arg1 : i32) -> i32 "None" {
- %cst = spirv.Constant 0: i32
- // expected-error @+1 {{result type mismatch: expected 'i32', but provided 'f32'}}
- %0 = spirv.FunctionCall @f_type_mismatch(%arg0, %arg0) : (i32, i32) -> f32
- spirv.ReturnValue %cst: i32
- }
-}
-
-// -----
-
spirv.module Logical GLSL450 {
spirv.func @f_foo(%arg0 : i32, %arg1 : i32) -> i32 "None" {
// expected-error @+1 {{op callee function 'f_undefined' not found in nearest symbol table}}
>From f0917d98592d4959e77cc5a912f20cef139c0f63 Mon Sep 17 00:00:00 2001
From: Erick Ochoa <erick.ochoalopez at amd.com>
Date: Wed, 17 Sep 2025 13:00:38 -0400
Subject: [PATCH 3/6] Add test case from issue
---
.../Dialect/SPIRV/IR/control-flow-ops.mlir | 29 +++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir b/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir
index f29f673b3dd3a..bfa33559fff61 100644
--- a/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir
+++ b/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir
@@ -220,6 +220,35 @@ spirv.module Logical GLSL450 {
// -----
+"builtin.module"() ({
+ "spirv.module"() <{
+ addressing_model = #spirv.addressing_model<Logical>,
+ memory_model = #spirv.memory_model<GLSL450>
+ }> ({
+ "spirv.func"() <{
+ function_control = #spirv.function_control<None>,
+ function_type = (f32) -> f32,
+ sym_name = "bar"
+ }> ({
+ ^bb0(%arg0: f32):
+ %0 = "spirv.FunctionCall"(%arg0) <{callee = @foo}> : (f32) -> f32
+ "spirv.ReturnValue"(%0) : (f32) -> ()
+ }) : () -> ()
+ // expected-error @+1 {{requires attribute 'function_type'}}
+ "spirv.func"() <{
+ function_control = #spirv.function_control<None>,
+ message = "2nd parent",
+ sym_name = "foo"
+ // This is invalid MLIR because function_type is missing from spirv.func
+ }> ({
+ ^bb0(%arg0: f32):
+ "spirv.ReturnValue"(%arg0) : (f32) -> ()
+ }) : () -> ()
+ }) : () -> ()
+}) : () -> ()
+
+// -----
+
//===----------------------------------------------------------------------===//
// spirv.mlir.loop
//===----------------------------------------------------------------------===//
>From 2ab414fc61b8ea3b2531ebd514de793557beb7df Mon Sep 17 00:00:00 2001
From: Erick Ochoa <erick.ochoalopez at amd.com>
Date: Thu, 18 Sep 2025 09:11:01 -0400
Subject: [PATCH 4/6] Revert "[mlir][spirv] Remove nonlocal call verification."
This reverts commit aed3836d1e30fc59c2da968541a4c58bbfcaed68.
---
mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp | 30 ++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp b/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp
index 95d63ddee6824..890406df74e72 100644
--- a/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp
@@ -160,12 +160,42 @@ LogicalResult FunctionCallOp::verify() {
<< fnName.getValue() << "' not found in nearest symbol table";
}
+ auto functionType = funcOp.getFunctionType();
+
if (getNumResults() > 1) {
return emitOpError(
"expected callee function to have 0 or 1 result, but provided ")
<< getNumResults();
}
+ if (functionType.getNumInputs() != getNumOperands()) {
+ return emitOpError("has incorrect number of operands for callee: expected ")
+ << functionType.getNumInputs() << ", but provided "
+ << getNumOperands();
+ }
+
+ for (uint32_t i = 0, e = functionType.getNumInputs(); i != e; ++i) {
+ if (getOperand(i).getType() != functionType.getInput(i)) {
+ return emitOpError("operand type mismatch: expected operand type ")
+ << functionType.getInput(i) << ", but provided "
+ << getOperand(i).getType() << " for operand number " << i;
+ }
+ }
+
+ if (functionType.getNumResults() != getNumResults()) {
+ return emitOpError(
+ "has incorrect number of results has for callee: expected ")
+ << functionType.getNumResults() << ", but provided "
+ << getNumResults();
+ }
+
+ if (getNumResults() &&
+ (getResult(0).getType() != functionType.getResult(0))) {
+ return emitOpError("result type mismatch: expected ")
+ << functionType.getResult(0) << ", but provided "
+ << getResult(0).getType();
+ }
+
return success();
}
>From 8c6d4c83c5566851d9c86d9c97ad1d7541baa1d1 Mon Sep 17 00:00:00 2001
From: Erick Ochoa <erick.ochoalopez at amd.com>
Date: Thu, 18 Sep 2025 09:23:20 -0400
Subject: [PATCH 5/6] Revert "Update tests"
This reverts commit bc34970cf59fe36b3e311b3f12985c62d75b5f03.
---
.../Dialect/SPIRV/IR/control-flow-ops.mlir | 42 +++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir b/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir
index bfa33559fff61..d25207ae6e922 100644
--- a/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir
+++ b/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir
@@ -210,6 +210,48 @@ spirv.module Logical GLSL450 {
// -----
+spirv.module Logical GLSL450 {
+ spirv.func @f_result_type_mismatch(%arg0 : i32, %arg1 : i32) -> () "None" {
+ // expected-error @+1 {{has incorrect number of results has for callee: expected 0, but provided 1}}
+ %1 = spirv.FunctionCall @f_result_type_mismatch(%arg0, %arg0) : (i32, i32) -> (i32)
+ spirv.Return
+ }
+}
+
+// -----
+
+spirv.module Logical GLSL450 {
+ spirv.func @f_type_mismatch(%arg0 : i32, %arg1 : i32) -> () "None" {
+ // expected-error @+1 {{has incorrect number of operands for callee: expected 2, but provided 1}}
+ spirv.FunctionCall @f_type_mismatch(%arg0) : (i32) -> ()
+ spirv.Return
+ }
+}
+
+// -----
+
+spirv.module Logical GLSL450 {
+ spirv.func @f_type_mismatch(%arg0 : i32, %arg1 : i32) -> () "None" {
+ %0 = spirv.Constant 2.0 : f32
+ // expected-error @+1 {{operand type mismatch: expected operand type 'i32', but provided 'f32' for operand number 1}}
+ spirv.FunctionCall @f_type_mismatch(%arg0, %0) : (i32, f32) -> ()
+ spirv.Return
+ }
+}
+
+// -----
+
+spirv.module Logical GLSL450 {
+ spirv.func @f_type_mismatch(%arg0 : i32, %arg1 : i32) -> i32 "None" {
+ %cst = spirv.Constant 0: i32
+ // expected-error @+1 {{result type mismatch: expected 'i32', but provided 'f32'}}
+ %0 = spirv.FunctionCall @f_type_mismatch(%arg0, %arg0) : (i32, i32) -> f32
+ spirv.ReturnValue %cst: i32
+ }
+}
+
+// -----
+
spirv.module Logical GLSL450 {
spirv.func @f_foo(%arg0 : i32, %arg1 : i32) -> i32 "None" {
// expected-error @+1 {{op callee function 'f_undefined' not found in nearest symbol table}}
>From 448179ad4bd662e68a07bde6416c7a36ba87086d Mon Sep 17 00:00:00 2001
From: Erick Ochoa <erick.ochoalopez at amd.com>
Date: Thu, 18 Sep 2025 09:28:41 -0400
Subject: [PATCH 6/6] Move FunctionCallOp verification to verifySymbolUses
---
.../Dialect/SPIRV/IR/SPIRVControlFlowOps.td | 4 +++-
mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp | 20 +++++++++++--------
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVControlFlowOps.td b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVControlFlowOps.td
index ef6682ab3630c..acb6467132be9 100644
--- a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVControlFlowOps.td
+++ b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVControlFlowOps.td
@@ -15,6 +15,7 @@
#define MLIR_DIALECT_SPIRV_IR_CONTROLFLOW_OPS
include "mlir/Dialect/SPIRV/IR/SPIRVBase.td"
+include "mlir/IR/SymbolInterfaces.td"
include "mlir/Interfaces/CallInterfaces.td"
include "mlir/Interfaces/ControlFlowInterfaces.td"
include "mlir/Interfaces/SideEffectInterfaces.td"
@@ -187,7 +188,8 @@ def SPIRV_BranchConditionalOp : SPIRV_Op<"BranchConditional", [
// -----
def SPIRV_FunctionCallOp : SPIRV_Op<"FunctionCall", [
- InFunctionScope, DeclareOpInterfaceMethods<CallOpInterface>]> {
+ InFunctionScope, DeclareOpInterfaceMethods<CallOpInterface>,
+ DeclareOpInterfaceMethods<SymbolUserOpInterface>]> {
let summary = "Call a function.";
let description = [{
diff --git a/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp b/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp
index 890406df74e72..f0b46e61965f4 100644
--- a/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/ControlFlowOps.cpp
@@ -151,10 +151,20 @@ LogicalResult BranchConditionalOp::verify() {
//===----------------------------------------------------------------------===//
LogicalResult FunctionCallOp::verify() {
+ if (getNumResults() > 1) {
+ return emitOpError(
+ "expected callee function to have 0 or 1 result, but provided ")
+ << getNumResults();
+ }
+ return success();
+}
+
+LogicalResult
+FunctionCallOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
auto fnName = getCalleeAttr();
- auto funcOp = dyn_cast_or_null<spirv::FuncOp>(
- SymbolTable::lookupNearestSymbolFrom((*this)->getParentOp(), fnName));
+ auto funcOp =
+ symbolTable.lookupNearestSymbolFrom<spirv::FuncOp>(*this, fnName);
if (!funcOp) {
return emitOpError("callee function '")
<< fnName.getValue() << "' not found in nearest symbol table";
@@ -162,12 +172,6 @@ LogicalResult FunctionCallOp::verify() {
auto functionType = funcOp.getFunctionType();
- if (getNumResults() > 1) {
- return emitOpError(
- "expected callee function to have 0 or 1 result, but provided ")
- << getNumResults();
- }
-
if (functionType.getNumInputs() != getNumOperands()) {
return emitOpError("has incorrect number of operands for callee: expected ")
<< functionType.getNumInputs() << ", but provided "
More information about the Mlir-commits
mailing list