Skip to content
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

[microNPU] Move optimization passes to be a module pass and ensure they are running #9831

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

lhutton1
Copy link
Contributor

@lhutton1 lhutton1 commented Jan 4, 2022

Moves LayoutOptimizer and LUTOptimizer passes to be a module pass, rather than a function pass. This is because it was found that these passes were not running in the NPU compilation flow. In addition, a test for both LayoutOptimizer and LUTOptimizer has been added to check that the passes are running in the compilation pipeline of the NPU.

cc @ekalda @manupa-arm

Copy link
Contributor

@ekalda ekalda left a comment

Choose a reason for hiding this comment

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

Thanks @lhutton1 for spotting and fixing this, it is a very subtle problem! As discussed offline, the fact that these passes silently ceased running as a part of a pipeline (they certainly did use to run when I was implementing the LUT pass) is quite worrying since it means there has been an underlying change to how a relay.transform.function_pass works (or maybe even a bug since the docstring of the implementation implies that it should work on the modules), so there could be other optimization passes that are silently not running now. I think @lhutton1 mentioned though that he didn't grep out many use cases of that function_pass outside of tutorials, but I think in that case we should properly retire it and update the tutorials. Re-tagging @manupa-arm for opinion...

@ekalda
Copy link
Contributor

ekalda commented Jan 11, 2022

(Forgot to say that dealing with function_pass is a stuff for other PRs, this one looks good to me!)

@lhutton1
Copy link
Contributor Author

Thanks @ekalda for taking a look, yes it seems quite strange to me. Just wanted to note that function_pass is used outside of tutorials, I was foolishly searching for relay.transform.function_pass rather than just function_pass :)

I did a little bit more digging and it seems like the function_pass doesn't get run on a function when there is a "Compiler" attribute associated with it. I wonder if this is intended behavior and if/when this changed?

@manupak
Copy link
Contributor

manupak commented Jan 11, 2022

kCompiler (a.k.a. "Compiler") attribute generally blocks most passes from visiting the function body. Are we saying there was a time where the pass worked even when the kCompiler is set ?

@ekalda
Copy link
Contributor

ekalda commented Jan 11, 2022

I saw the LUT pass working as a part of a pipeline when I was developing it (I first developed it by running an end to end codegen test with a breakpoint before the pass and checked that the Relay had changed as expected after stepping over it). Out of interest, why does the kCompiler attribute block the passes visiting the function body?

@manupak
Copy link
Contributor

manupak commented Jan 11, 2022

It is used to annotate relay functions meant to be compiled with BYOC. Therefore, the compilation passes of core compiler ignores it. IIRC, that was the reason.

I believe this affects function pass because (and not module pass) the pass it self does not choose which functions to mutate as opposed to a module pass.

@lhutton1
Copy link
Contributor Author

Thanks @manupa-arm, in that case it sounds like migrating to a module pass is the correct thing to do here. It's still a bit of a mystery as to why @ekalda and I believe this used to work, although its probably worth not dwelling on it for now

Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

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

The implementation LGTM!

I think we need to be a specific with the title and/or commit message to say what this PR enables. I believe that should be :

  1. Moves LUTOptimizer to be a module pass
  2. Ensure LUTOptimizer Pass is run

rather than generically adding the feature to ensure all optimization passes are run.

@lhutton1
Copy link
Contributor Author

Agreed, that makes more sense :)

are running

Moves LayoutOptimizer and LUTOptimizer passes to be a module pass,
rather than a function pass. This is because it was found that these
passes were not running in the NPU compilation flow. In addition, a
test for both LayoutOptimizer and LUTOptimizer has been added to check
that the passes are running in the compilation pipeline of the NPU.

Change-Id: I5145c6f02eeb0daea3cdba56198e0804ec32f351
@lhutton1 lhutton1 changed the title [microNPU] Ensure optimization passes are running in compilation pipeline [microNPU] Move optimization passes to be a module pass and ensure they are running Jan 18, 2022
@lhutton1
Copy link
Contributor Author

friendly ping for approval/comments

Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

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

LGTM!

@manupak manupak merged commit e390d9e into apache:main Jan 20, 2022
@manupak
Copy link
Contributor

manupak commented Jan 20, 2022

Thanks @lhutton1 @ekalda !

@lhutton1 lhutton1 deleted the check-optimize branch January 20, 2022 18:51
yuanfz98 pushed a commit to yuanfz98/tvm that referenced this pull request Jan 24, 2022
…ey (apache#9831)

are running

Moves LayoutOptimizer and LUTOptimizer passes to be a module pass,
rather than a function pass. This is because it was found that these
passes were not running in the NPU compilation flow. In addition, a
test for both LayoutOptimizer and LUTOptimizer has been added to check
that the passes are running in the compilation pipeline of the NPU.

Change-Id: I5145c6f02eeb0daea3cdba56198e0804ec32f351
ylc pushed a commit to ylc/tvm that referenced this pull request Feb 16, 2022
…ey (apache#9831)

are running

Moves LayoutOptimizer and LUTOptimizer passes to be a module pass,
rather than a function pass. This is because it was found that these
passes were not running in the NPU compilation flow. In addition, a
test for both LayoutOptimizer and LUTOptimizer has been added to check
that the passes are running in the compilation pipeline of the NPU.

Change-Id: I5145c6f02eeb0daea3cdba56198e0804ec32f351
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants