[MLIR] RemoveDeadValues: Allowing IRs with global constants to get dead values removed #116519

parsifal-47 · 2024-11-17T01:47:38Z

This change is related to discussion:
https://discourse.llvm.org/t/question-on-criteria-for-acceptable-ir-in-removedeadvaluespass/83131

I do not know the original reason to disallow the optimization on modules with global private constant. Please let me know what am I missing, I will be happy to make it better. Thank you!

CC: @Wheest

github-actions · 2024-11-17T01:47:59Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-11-17T01:48:34Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Renat Idrisov (parsifal-47)

Changes

This change is related to discussion:
https://discourse.llvm.org/t/question-on-criteria-for-acceptable-ir-in-removedeadvaluespass/83131

I do not know the original reason to disallow the optimization on modules with global private constant. Please let me know what am I missing, I will be happy to make it better. Thank you!

CC: @Wheest

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

2 Files Affected:

(modified) mlir/lib/Transforms/RemoveDeadValues.cpp (-1)
(modified) mlir/test/Transforms/remove-dead-values.mlir (+3-3)

diff --git a/mlir/lib/Transforms/RemoveDeadValues.cpp b/mlir/lib/Transforms/RemoveDeadValues.cpp
index 7e45f18b660ba7..edfb1969ab75c7 100644
--- a/mlir/lib/Transforms/RemoveDeadValues.cpp
+++ b/mlir/lib/Transforms/RemoveDeadValues.cpp
@@ -579,7 +579,6 @@ void RemoveDeadValues::runOnOperation() {
     if (op == module)
       return WalkResult::advance();
     if (isa<BranchOpInterface>(op) ||
-        (isa<SymbolOpInterface>(op) && !isa<FunctionOpInterface>(op)) ||
         (isa<SymbolUserOpInterface>(op) && !isa<CallOpInterface>(op))) {
       op->emitError() << "cannot optimize an IR with non-function symbol ops, "
                          "non-call symbol user ops or branch ops\n";
diff --git a/mlir/test/Transforms/remove-dead-values.mlir b/mlir/test/Transforms/remove-dead-values.mlir
index 9f2be3331b6b4b..b06b3320706cc8 100644
--- a/mlir/test/Transforms/remove-dead-values.mlir
+++ b/mlir/test/Transforms/remove-dead-values.mlir
@@ -1,12 +1,12 @@
 // 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 inside the module (const @__dont_touch_unacceptable_ir).
+// The IR is updated regardless of memref.global private constant
 //
 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 {
+    %0 = tensor.empty() : tensor<10xbf16>
+    // CHECK-NOT: tensor.empty
     return %arg0 : i32
   }
 }

github-actions · 2024-11-17T01:49:23Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

codemzs · 2024-11-17T18:52:45Z

@srishti-pm Can you please review this change? I am curious why the IR is not considered acceptable if it contains something like memref.global op?

Wheest

From my perspective it should be fine if IR with named symbols like constants use this pass (my recent PR did this for named modules #109990).

A justification for keeping the existing behaviour could be: "that which has a name should be handled more carefully, and not removed unless specifically requested."

However, since this is a blocker for some of my own work, I don't support it, and think this PR should go through.

That being said, I could imagine adding this behaviour back in with --remove-dead-values="remove-user-symbols"

Wheest · 2024-11-18T10:00:40Z

mlir/lib/Transforms/RemoveDeadValues.cpp

@@ -579,7 +579,6 @@ void RemoveDeadValues::runOnOperation() {
    if (op == module)
      return WalkResult::advance();
    if (isa<BranchOpInterface>(op) ||
-        (isa<SymbolOpInterface>(op) && !isa<FunctionOpInterface>(op)) ||
        (isa<SymbolUserOpInterface>(op) && !isa<CallOpInterface>(op))) {
      op->emitError() << "cannot optimize an IR with non-function symbol ops, "


So the original comment above says:

// The removal of non-live values is performed iff there are no branch ops, // all symbol ops present in the IR are function-like, and all symbol user ops // present in the IR are call-like.

and the error message includes non-call symbol user ops. With this PR I believe that IR with non-call symbol user ops are no longer excluded?

Perhaps the error message/comment should be updated to reflect that.

updated both the comment and error message, thank you!

Wheest · 2024-11-18T10:00:44Z

mlir/test/Transforms/remove-dead-values.mlir

 //
 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>


is @__dont_touch_unacceptable_ir still a meaningful name here, since we expect the IR to be updated?

thank you, done!

River707 · 2024-11-18T17:54:31Z

From my perspective I'd love to see all of the acceptableIR checks removed completely. This passes presents itself as some general aggressive DCE, but is currently only applicable in a very limited number of situations. I'd support any form of additions that move it closer towards a more general pass.

codemzs · 2024-11-19T17:20:01Z

@River707 @Wheest Who is the right person to ask to commit this change?

codemzs · 2024-11-20T04:11:06Z

Hello @joker-eph

It appears that you have recently reviewed and committed changes to this file. Would it be possible for you to review this change?

joker-eph · 2024-11-20T19:46:45Z

mlir/lib/Transforms/RemoveDeadValues.cpp

  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))) {


This last check is also not clear to me. But I'm not sure if this has all to do with some aspects of the dataflow framework?

joker-eph

It's not clear why this check is in-place (unfortunately the comment documents the "what" and not the "why"...).

In any case we could tighten this back with motivating examples, if they show up, and I suspect the checks could be more targeted: for example instead of disabling the pass entirely, we could disable it on a per-function basis.

codemzs · 2024-11-22T16:20:06Z

It's not clear why this check is in-place (unfortunately the comment documents the "what" and not the "why"...).

In any case we could tighten this back with motivating examples, if they show up, and I suspect the checks could be more targeted: for example instead of disabling the pass entirely, we could disable it on a per-function basis.

@joker-eph I was curious if there is anything else needed for this change to be committed?

joker-eph · 2024-11-22T16:27:54Z

It is approved, and the author has commit access, so I didn't feel like committing in their place. @parsifal-47 what's up?

parsifal-47 · 2024-11-22T17:44:16Z

hmm, I see the following:
"This branch has no conflicts with the base branch
Only those with write access to this repository can merge pull requests."

parsifal-47 · 2024-11-22T18:01:25Z

sorry, maybe I do not understand the workflow, the PR is approved, should I look for motivating examples?
I was thinking that we are waiting for all potentially interested people to see it before it gets merged.

codemzs · 2024-11-22T19:34:44Z

It is approved, and the author has commit access, so I didn't feel like committing in their place. @parsifal-47 what's up?

@joker-eph Based on my offline conversation with @parsifal-47 it appears he lacks write access. Would it be possible for you to commit and merge this change into llvm:main? Thank you.

github-actions · 2024-11-22T22:42:43Z

@parsifal-47 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

…Pass (#117405) This change removes the restriction on `SymbolUserOpInterface` operators so they can be used with operators that implement `SymbolOpInterface`, example: `memref.global` implements `SymbolOpInterface` so it can be used with `memref.get_global` which implements `SymbolUserOpInterface` ``` // Define a global constant array memref.global "private" constant @global_array : memref<10xi32> = dense<[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]> : tensor<10xi32> // Access this global constant within a function func @use_global() { %0 = memref.get_global @global_array : memref<10xi32> } ``` Reference: #116519 and https://discourse.llvm.org/t/question-on-criteria-for-acceptable-ir-in-removedeadvaluespass/83131 --------- Co-authored-by: Zeeshan Siddiqui <[email protected]>

llvmbot added mlir:core MLIR Core Infrastructure mlir labels Nov 17, 2024

Wheest reviewed Nov 18, 2024

View reviewed changes

parsifal-47 added 2 commits November 18, 2024 14:48

Allowing IRs with global constants to get dead values removed

0247d1b

Address feedback from Code Review

c145899

parsifal-47 force-pushed the main branch from 75a6efb to c145899 Compare November 18, 2024 22:56

joker-eph reviewed Nov 20, 2024

View reviewed changes

joker-eph approved these changes Nov 20, 2024

View reviewed changes

sstamenova merged commit 24ced77 into llvm:main Nov 22, 2024
8 checks passed

codemzs mentioned this pull request Nov 23, 2024

Allow SymbolUserOpInterface operators to be used in RemoveDeadValues Pass #117405

Merged

[MLIR] RemoveDeadValues: Allowing IRs with global constants to get dead values removed #116519

[MLIR] RemoveDeadValues: Allowing IRs with global constants to get dead values removed #116519

Uh oh!

Conversation

parsifal-47 commented Nov 17, 2024

Uh oh!

github-actions bot commented Nov 17, 2024

Uh oh!

llvmbot commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 17, 2024

Uh oh!

codemzs commented Nov 17, 2024

Uh oh!

Wheest left a comment

Choose a reason for hiding this comment

Uh oh!

Wheest Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

parsifal-47 Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

Wheest Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

parsifal-47 Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

River707 commented Nov 18, 2024

Uh oh!

codemzs commented Nov 19, 2024

Uh oh!

codemzs commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joker-eph Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

joker-eph left a comment

Choose a reason for hiding this comment

Uh oh!

codemzs commented Nov 22, 2024

Uh oh!

joker-eph commented Nov 22, 2024

Uh oh!

parsifal-47 commented Nov 22, 2024

Uh oh!

parsifal-47 commented Nov 22, 2024

Uh oh!

codemzs commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Nov 22, 2024

Uh oh!

Uh oh!

llvmbot commented Nov 17, 2024 •

edited

Loading

codemzs commented Nov 20, 2024 •

edited

Loading

codemzs commented Nov 22, 2024 •

edited

Loading