Skip to content

[mlir] Fix remove-dead-values pass throws error when module has a name #109990

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.

Sign up for GitHub

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 7 commits into from
Oct 3, 2024
Merged

[mlir] Fix remove-dead-values pass throws error when module has a name #109990

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eeba53c
Fix remove-dead-values throws error w/module name
Wheest Sep 25, 2024
94b7dd4
Test for non-function symbol op inside the module
Wheest Sep 26, 2024
3ad0970
Merge branch 'main' into mlir-named-module-error
Wheest Sep 27, 2024
ca40edd
Merge branch 'main' into mlir-named-module-error
Wheest Sep 27, 2024
df31722
Simplify check condition on remove-dead-value test
Wheest Oct 3, 2024
d60b194
Merge branch 'mlir-named-module-error' of https://github.com/Wheest/l…
Wheest Oct 3, 2024
d4538f2
Merge branch 'main' into mlir-named-module-error
Wheest Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions mlir/lib/Transforms/RemoveDeadValues.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,8 @@ void RemoveDeadValues::runOnOperation() {
// all symbol ops present in the IR are function-like, and all symbol user ops
// present in the IR are call-like.
WalkResult acceptableIR = module->walk([&](Operation *op) {
if (op == module)
return WalkResult::advance();
if (isa<BranchOpInterface>(op) ||
(isa<SymbolOpInterface>(op) && !isa<FunctionOpInterface>(op)) ||
(isa<SymbolUserOpInterface>(op) && !isa<CallOpInterface>(op))) {
Expand Down
22 changes: 17 additions & 5 deletions mlir/test/Transforms/remove-dead-values.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,25 @@
// RUN: mlir-opt %s -remove-dead-values -split-input-file -verify-diagnostics | FileCheck %s

// The IR remains untouched because of the presence of a non-function-like
// symbol op (module @dont_touch_unacceptable_ir).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have still a test with a non-function symbol op inside the module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 2nd test checks for isa<BranchOpInterface>(op) and raises the error, but no we don't have a test for the "non-function symbol op inside the module" condition (I don't think we had one for this case before).

In terms of non-function symbol ops, would a global constant make sense? I've added this test which meets the requirements:

module {
// expected-error @+1 {{cannot optimize an IR with non-function symbol ops, non-call symbol user ops or branch ops}}
  memref.global "private" constant @__dont_touch_unacceptable_ir : memref<i32> = dense<0>
  func.func @main(%arg0: i32) -> i32 {
    return %arg0 : i32
  }
}

// symbol op inside the module (const @__dont_touch_unacceptable_ir).
//
module {
// expected-error @+1 {{cannot optimize an IR with non-function symbol ops, non-call symbol user ops or branch ops}}
module @dont_touch_unacceptable_ir {
func.func @has_cleanable_simple_op(%arg0 : i32) {
%non_live = arith.addi %arg0, %arg0 : i32
return
memref.global "private" constant @__dont_touch_unacceptable_ir : memref<i32> = dense<0>
func.func @main(%arg0: i32) -> i32 {
return %arg0 : i32
}
}

// -----

// Dead values are removed from the IR even if the module has a name
//
module @named_module_acceptable {
func.func @main(%arg0: tensor<10xf32>) -> tensor<10xf32> {
%0 = tensor.empty() : tensor<10xbf16>
// CHECK-NOT: %[[C:.*]] = tensor.empty[[C:.*]]
return %arg0 : tensor<10xf32>
}
}

Expand Down
Loading