Skip to content

[REFACTOR][BOYC] Non recursive partitioning - #5493

Merged
masahi merged 5 commits into
apache:masterfrom
zhiics:non-recursive-partition
May 1, 2020
Merged

[REFACTOR][BOYC] Non recursive partitioning#5493
masahi merged 5 commits into
apache:masterfrom
zhiics:non-recursive-partition

Conversation

@zhiics

@zhiicszhiics commented Apr 30, 2020

Copy link
Copy Markdown
Member

This PR refactors the partitioning pass by using non-recursive mutator. It also removes the unnecessary mutators as we only need to look at begin/end annotations which are definitely wrapped in call nodes. In addition, a metadata struct is used to maintain the intermediate data needed for partitioning.

@zhiics

zhiics commented Apr 30, 2020

Copy link
Copy Markdown
MemberAuthor

@mbrookhart please take a look for the mixed mutator pattern. BTW, we will still need to refactor the infertype pass as it is the most frequently used pass.

@comaniac@masahi@mbaret@manupa-arm@trevor-m please take a look.

comaniac
comaniac approved these changes Apr 30, 2020
View reviewed changes

@comaniaccomaniac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We were trying rewriter but it is not applicable to this pass due to https://github.com/apache/incubator-tvm/pull/5493/files#diff-8d2cdf6314f73e4b32892679ad4dc44aR280, which traverses other nodes out of order. As a result, the current solution seems the most suitable.

mbrookhart
mbrookhart approved these changes Apr 30, 2020
View reviewed changes

@mbrookhartmbrookhart left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use of the iterative traversals looks great. Since you originally authored the class, I'll believe you about the necessary mutators.

Why so much auto-formatting noise? As a larger conversation, we might want to build a style checker into CI that enforces a particular auto-format implementaiton...

@comaniac

comaniac commented Apr 30, 2020

Copy link
Copy Markdown
Contributor

Ah I think that's because I manually ran clang-format for the file. We should definitely build style checker in CI.

manupak
manupak reviewed May 1, 2020
View reviewed changes
Comment threadsrc/relay/transforms/partition_graph.cc
@masahimasahi self-assigned this May 1, 2020
masahi
masahi reviewed May 1, 2020
View reviewed changes
Comment threadsrc/relay/transforms/partition_graph.cc Outdated
masahi
masahi reviewed May 1, 2020
View reviewed changes
Comment threadsrc/relay/transforms/partition_graph.cc Outdated
masahi
masahi reviewed May 1, 2020
View reviewed changes
Comment threadsrc/relay/transforms/partition_graph.cc Outdated
bool found_start_{false};
bool found_end_{false};
/*! \brief Map from each region output expr node to its output index and TupleGetItem node. */
std::unordered_map<Expr, std::pair<int, TupleGetItem>, ObjectHash, ObjectEqual> out_expr_indices;

@masahimasahiMay 1, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since both arguments of TupleGetItem (func_call and the index) are in this struct, this TupleGetItem seems redundant to me. Also see my comment at L282.

@masahimasahiMay 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this TupleGetItem is intended to be cached, please come up with a better name than out_expr_indices, since it is not just indices.

@comaniaccomaniacMay 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rename to region_func_out and only cache the function output expressions (Call or TupleGetItem).

masahi
masahi reviewed May 1, 2020
View reviewed changes
Comment threadsrc/relay/transforms/partition_graph.cc Outdated
masahi
masahi reviewed May 1, 2020
View reviewed changes
Comment threadsrc/relay/transforms/partition_graph.cc Outdated
masahi
masahi reviewed May 1, 2020
View reviewed changes
Comment threadsrc/relay/transforms/partition_graph.cc Outdated
masahi
masahi reviewed May 1, 2020
View reviewed changes
Comment threadsrc/relay/transforms/partition_graph.cc Outdated
masahi
masahi reviewed May 1, 2020
View reviewed changes
Comment threadsrc/relay/transforms/partition_graph.cc Outdated
@comaniac
comaniacforce-pushed the non-recursive-partition branch from 3a6f409 to 00be9a9CompareMay 1, 2020 18:04
@comaniac
comaniacforce-pushed the non-recursive-partition branch from 00be9a9 to 26404c1CompareMay 1, 2020 18:08
@comaniac

comaniac commented May 1, 2020

Copy link
Copy Markdown
Contributor

@masahi thanks for valuable suggestions. We've refactored CreateFunction to create all TupleGetItem nodes so that GetFunctionOutput can be safely removed and the metadata could be more concise. PTAL.

@masahi
masahi merged commit 9c1e74c into apache:masterMay 1, 2020
@masahi

masahi commented May 1, 2020

Copy link
Copy Markdown
Member

Thanks @zhiics@comaniac@mbrookhart@manupa-arm

@zhiics
zhiics deleted the non-recursive-partition branch May 1, 2020 22:04
Laurawly pushed a commit to Laurawly/tvm-1 that referenced this pull request May 9, 2020
@zhiics@comaniac@Laurawly
[REFACTOR][BOYC] Non recursive partitioning (apache#5493)
7a853b9
* non recursive partitioning
* refactor maps
* rebase upstream
* refactor shared output
* address comments
Co-authored-by: Cody Yu <comaniac0422@gmail.com>
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 9, 2020
@zhiics@comaniacTrevor Morris
[REFACTOR][BOYC] Non recursive partitioning (apache#5493)
4e94fd9
* non recursive partitioning
* refactor maps
* rebase upstream
* refactor shared output
* address comments
Co-authored-by: Cody Yu <comaniac0422@gmail.com>
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 18, 2020
@zhiics@comaniacTrevor Morris
[REFACTOR][BOYC] Non recursive partitioning (apache#5493)
c3c385e
* non recursive partitioning
* refactor maps
* rebase upstream
* refactor shared output
* address comments
Co-authored-by: Cody Yu <comaniac0422@gmail.com>
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jun 18, 2020
@zhiics@comaniac@trevor-m
[REFACTOR][BOYC] Non recursive partitioning (apache#5493)
27e24df
* non recursive partitioning
* refactor maps
* rebase upstream
* refactor shared output
* address comments
Co-authored-by: Cody Yu <comaniac0422@gmail.com>
@ZihengJiangZihengJiang mentioned this pull request Sep 25, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@comaniaccomaniaccomaniac approved these changes
@manupakmanupakmanupak left review comments
@masahimasahimasahi approved these changes
@mbrookhartmbrookhartmbrookhart approved these changes

Assignees

@masahimasahi

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zhiics@comaniac@masahi@mbrookhart@manupak