Skip to content

[mlir][transforms] Use isExternal instead of isDeclaration for FunctionOpInterface #116573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 4, 2024

Conversation

CoTinker
Copy link
Contributor

This PR fixes a bug in RemoveDeadValues where the FunctionOpInterface does not have the isDeclaration method. As a result, we should use the isExternal method instead. Fixes #116347.

…unctionOpInterface`

This PR fixes a bug in `RemoveDeadValues` where the `FunctionOpInterface` does not have the `isDeclaration` method. As a result, we should use the `isExternal` method instead.
@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Nov 18, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 18, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Longsheng Mou (CoTinker)

Changes

This PR fixes a bug in RemoveDeadValues where the FunctionOpInterface does not have the isDeclaration method. As a result, we should use the isExternal method instead. Fixes #116347.


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

2 Files Affected:

  • (modified) mlir/lib/Transforms/RemoveDeadValues.cpp (+2-2)
  • (modified) mlir/test/Transforms/remove-dead-values.mlir (+5)
diff --git a/mlir/lib/Transforms/RemoveDeadValues.cpp b/mlir/lib/Transforms/RemoveDeadValues.cpp
index 7e45f18b660ba7..9f942485711297 100644
--- a/mlir/lib/Transforms/RemoveDeadValues.cpp
+++ b/mlir/lib/Transforms/RemoveDeadValues.cpp
@@ -191,10 +191,10 @@ static void cleanSimpleOp(Operation *op, RunLivenessAnalysis &la) {
 ///   non-live across all callers),
 ///   (5) Dropping the uses of these return values from its callers, AND
 ///   (6) Erasing these return values
-/// iff it is not public or declaration.
+/// iff it is not public or external.
 static void cleanFuncOp(FunctionOpInterface funcOp, Operation *module,
                         RunLivenessAnalysis &la) {
-  if (funcOp.isPublic() || funcOp.isDeclaration())
+  if (funcOp.isPublic() || funcOp.isExternal())
     return;
 
   // Get the list of unnecessary (non-live) arguments in `nonLiveArgs`.
diff --git a/mlir/test/Transforms/remove-dead-values.mlir b/mlir/test/Transforms/remove-dead-values.mlir
index 9f2be3331b6b4b..edb100acf3849b 100644
--- a/mlir/test/Transforms/remove-dead-values.mlir
+++ b/mlir/test/Transforms/remove-dead-values.mlir
@@ -374,3 +374,8 @@ func.func @kernel(%arg0: memref<18xf32>) {
 
 // CHECK: func.func private @no_block_func_declaration()
 func.func private @no_block_func_declaration() -> ()
+
+// -----
+
+// CHECK: llvm.func @no_block_external_func()
+llvm.func @no_block_external_func() attributes {sym_visibility = "private"}

@CoTinker
Copy link
Contributor Author

Ping~

@CoTinker CoTinker requested a review from eric-k256 November 28, 2024 08:52
@CoTinker
Copy link
Contributor Author

CoTinker commented Dec 2, 2024

ping~

Copy link
Contributor

@hstk30-hw hstk30-hw left a comment

Choose a reason for hiding this comment

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

LGTM

@hstk30-hw hstk30-hw merged commit e08e5e2 into llvm:main Dec 4, 2024
11 checks passed
@CoTinker CoTinker deleted the func branch December 4, 2024 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MLIR]-remove-dead-values triggers Assertion Failure `results.size() == 1 && "expected a single result type"'
3 participants