Skip to content

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