[mlir][spirv] Use verifySymbolUses for spirv.FunctionCall.
#159399
Merged
+44
−9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`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
amd-eochoalo
commented
Sep 17, 2025
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-spirv Author: Erick Ochoa Lopez (amd-eochoalo) Changes
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, Fixes #159295 Full diff: https://github.com/llvm/llvm-project/pull/159399.diff 2 Files Affected:
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();
}
diff --git a/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir b/mlir/test/Dialect/SPIRV/IR/control-flow-ops.mlir
index 8ec0bf5bbaacf..bfa33559fff61 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}}
@@ -262,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
//===----------------------------------------------------------------------===//
|
amd-eochoalo
commented
Sep 17, 2025
Comment on lines
223
to
251
| "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) -> () | ||
| }) : () -> () | ||
| }) : () -> () | ||
| }) : () -> () | ||
|
|
||
| // ----- | ||
|
|
There was a problem hiding this comment.
Suggested change
| "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) -> () | |
| }) : () -> () | |
| }) : () -> () | |
| }) : () -> () | |
| // ----- |
I think removing this test is also reasonable. Happy to make this change if reviewers agree.
There was a problem hiding this comment.
@kuhar, can I interpret your thumbs up as "remove this test"? (Got confused by your correction to the comment inside this test.)
There was a problem hiding this comment.
I will merge then but happy to delete this test in another PR.
amd-eochoalo
changed the title
verifySymbolUses for spirv.FunctionCall.
kuhar
approved these changes
Sep 18, 2025
Co-authored-by: Jakub Kuderski <kubakuderski@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
The fix includes adding the
SymbolUserOpInterfacetoFunctionCalland moving most of the verification logic toverifySymbolUses.Fixes #159295