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] Add a pass to reorder copy and compute nodes #10959

Merged

Conversation

NicolaLancellotti
Copy link
Contributor

@NicolaLancellotti NicolaLancellotti commented Apr 11, 2022

This pr adds a pass to reorder Arm(R) Ethos(TM)-U copy and compute nodes in such a way that independent DMA copies, and computes happen in parallel.

cc @lhutton1 @ekalda @manupa-arm

@github-actions github-actions bot requested a review from manupak April 11, 2022 09:44
Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

Thanks @NicolaLancellotti! Looks good overall, just some small things below

src/tir/contrib/ethosu/passes.cc Outdated Show resolved Hide resolved
src/tir/contrib/ethosu/passes.cc Outdated Show resolved Hide resolved
src/tir/contrib/ethosu/passes.cc Show resolved Hide resolved
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.

Looks really good, @NicolaLancellotti! I'd be interested in seeing how that pass interacts with graphs that have mixture of operators with and without weights, e.g. it seems to me that when we have a graph that looks like
pooling -> copy -> copy -> conv2d -> copy -> copy -> depthwise2d
it will end up after this pass as
copy -> copy -> pooling -> copy -> copy -> conv2d ...
I suppose that is intentional, that we start copying the conv2d weights in while the MAC engine is crunching the pooling? Maybe it's worth adding a test that exercises that kind of mixture of ops?

src/tir/contrib/ethosu/passes.cc Outdated Show resolved Hide resolved
@NicolaLancellotti NicolaLancellotti force-pushed the passes/copy_compute_reordering branch from 63b6752 to 78f05ea Compare April 12, 2022 15:05
@NicolaLancellotti
Copy link
Contributor Author

I'd be interested in seeing how that pass interacts with graphs that have mixture of operators with and without weights, e.g. it seems to me that when we have a graph that looks like
pooling -> copy -> copy -> conv2d -> copy -> copy -> depthwise2d
it will end up after this pass as
copy -> copy -> pooling -> copy -> copy -> conv2d ...
I suppose that is intentional, that we start copying the conv2d weights in while the MAC engine is crunching the pooling? Maybe it's worth adding a test that exercises that kind of mixture of ops?

Yes, it is intentional, and the reordering is just what you said. I have added a test too.

@lhutton1
Copy link
Contributor

Nice thanks @NicolaLancellotti! It looks like we need a re-trigger due to the linting issues CI had last week :)

@NicolaLancellotti NicolaLancellotti force-pushed the passes/copy_compute_reordering branch 2 times, most recently from 74be4ce to 88c0ea3 Compare April 20, 2022 14:56
Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@lhutton1
Copy link
Contributor

Looks like it now conflicts with #10344 :/

@NicolaLancellotti NicolaLancellotti force-pushed the passes/copy_compute_reordering branch from 88c0ea3 to a1f8b5b Compare April 22, 2022 11:44
@NicolaLancellotti NicolaLancellotti force-pushed the passes/copy_compute_reordering branch from a1f8b5b to e42b246 Compare April 29, 2022 16:31
@NicolaLancellotti
Copy link
Contributor Author

I have added a parameter to specify the maximum number of movements allowed for a copy.

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.

Thanks @NicolaLancellotti for the great work here.

I've left some comments to improve the code readability and a suggestion to allow PassContext to configure the pass.

src/tir/contrib/ethosu/passes.cc Show resolved Hide resolved
src/tir/contrib/ethosu/passes.cc Outdated Show resolved Hide resolved
src/tir/contrib/ethosu/passes.cc Outdated Show resolved Hide resolved
src/tir/contrib/ethosu/passes.cc Outdated Show resolved Hide resolved
src/tir/contrib/ethosu/passes.cc Outdated Show resolved Hide resolved
src/tir/contrib/ethosu/passes.cc Outdated Show resolved Hide resolved
src/tir/contrib/ethosu/passes.cc Outdated Show resolved Hide resolved
src/tir/contrib/ethosu/passes.cc Outdated Show resolved Hide resolved
src/tir/contrib/ethosu/passes.cc Show resolved Hide resolved
@NicolaLancellotti NicolaLancellotti force-pushed the passes/copy_compute_reordering branch from 017c855 to ce772d6 Compare May 4, 2022 10:05
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!

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.

Ah just found one thing that needs to be fixed.
Happy to take an immediate follow up. Let me know

src/tir/contrib/ethosu/passes.cc Outdated Show resolved Hide resolved
@NicolaLancellotti NicolaLancellotti force-pushed the passes/copy_compute_reordering branch from ce772d6 to 367cdc9 Compare May 6, 2022 09:19
@NicolaLancellotti NicolaLancellotti force-pushed the passes/copy_compute_reordering branch 2 times, most recently from 12f7c44 to 5da51da Compare May 17, 2022 16:11
@NicolaLancellotti NicolaLancellotti force-pushed the passes/copy_compute_reordering branch from 5da51da to c82ea02 Compare May 18, 2022 08:22
@lhutton1 lhutton1 merged commit 1b32245 into apache:main May 18, 2022
@lhutton1
Copy link
Contributor

Thanks @NicolaLancellotti, @manupa-arm, @ekalda!

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.

4 participants