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

[BYOC] add multi functions support in partition pass #8464

Merged
merged 4 commits into from
Jul 16, 2021

Conversation

zxy844288792
Copy link
Contributor

If a module contains multiple functions, the partition pass fail with this kind of message: Check failed: (!module_->ContainGlobalVar(fname)) is false: Global function ccompiler_0 already exists

The reason of that is due to for each function in the module we maintain a AnnotatedRegion object and reset the region_id which region_id is used to form the subfunction name. Here introduces the new naming which will also use the function name to form the final subfunction name.

Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

src/relay/analysis/annotated_region_set.cc Outdated Show resolved Hide resolved
src/relay/analysis/annotated_region_set.cc Outdated Show resolved Hide resolved
src/relay/transforms/partition_graph.cc Outdated Show resolved Hide resolved
@comaniac comaniac self-assigned this Jul 13, 2021
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

LGTM
@leandron the tests in Ethos-N are failed due to the change of function names by this PR. I asked @zxy844288792 to disable them to unblock the CI. Please feel free to re-enable them later on.

@leandron
Copy link
Contributor

LGTM
@leandron the tests in Ethos-N are failed due to the change of function names by this PR. I asked @zxy844288792 to disable them to unblock the CI. Please feel free to re-enable them later on.

Thanks for letting me know. We’ll have a look tomorrow. 👍🏻

@Leo-arm
Copy link
Contributor

Leo-arm commented Jul 16, 2021

I'll have a look. If we can hold off momentary I might be able to fix this patch.

@Leo-arm
Copy link
Contributor

Leo-arm commented Jul 16, 2021

The hashes need to be updated. I got most of the way through that; I will finish this on Monday.

@comaniac comaniac merged commit c8b9900 into apache:main Jul 16, 2021
@comaniac
Copy link
Contributor

Thanks @zxy844288792.
@Leo-arm Right. The hash has to be updated when a PR changes the graph (partitioned function names in this case). It's actually not a good practice as people need to call out ARM folks every time. It would be better if we can improve these tests.

zxy844288792 pushed a commit to zxy844288792/tvm that referenced this pull request Jul 26, 2021
* add support for multi function

* address commits and fix lint

* fix testcases and using a set to avoid duplicate func name

* fix lint
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jul 26, 2021
* add support for multi function

* address commits and fix lint

* fix testcases and using a set to avoid duplicate func name

* fix lint
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
* add support for multi function

* address commits and fix lint

* fix testcases and using a set to avoid duplicate func name

* fix lint
zxy844288792 pushed a commit to zxy844288792/tvm that referenced this pull request Mar 4, 2022
* add support for multi function

* address commits and fix lint

* fix testcases and using a set to avoid duplicate func name

* fix lint
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