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

[TVMC] use target_host when it is set #6855

Merged
merged 7 commits into from
Dec 1, 2020
Merged

[TVMC] use target_host when it is set #6855

merged 7 commits into from
Dec 1, 2020

Conversation

euntaik
Copy link
Contributor

@euntaik euntaik commented Nov 5, 2020

Feed target_host to the relay.build when it is provided.

Copy link
Contributor

@giuseros giuseros left a comment

Choose a reason for hiding this comment

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

I like this change.Could you add a test to verify this behavior?

@comaniac comaniac added status: need update need update based on feedbacks status: need test case need test cases to cover the change and removed status: need update need update based on feedbacks labels Nov 5, 2020
@comaniac comaniac self-assigned this Nov 7, 2020
@comaniac
Copy link
Contributor

@euntaik please address the review comments.

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.

Just a nit.

The test case looks not relevant at the first glance, but I guess maybe OpenCL is the more obvious backend that requires target host to be set explicitly?

Comment on lines +166 to +170
# check for output types
assert type(graph) is str
assert type(lib) is tvm.runtime.module.Module
assert type(params) is dict
assert type(dumps) is dict
Copy link
Contributor

@comaniac comaniac Nov 20, 2020

Choose a reason for hiding this comment

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

Those checks look not cover anything special? To me, as long as you can compile the model without errors, then you should have used the right target host.

@comaniac comaniac merged commit 93758ca into apache:main Dec 1, 2020
@comaniac
Copy link
Contributor

comaniac commented Dec 1, 2020

Thanks @euntaik @giuseros

@comaniac comaniac added status: accepted and removed status: need test case need test cases to cover the change labels Dec 1, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 2, 2020
* [TVMC] add cl support in tvmc runner

* [TVMC] use target_host when it is set

* Cleanup comment and asssert device type in else case

* add a test for tvmc compiler

* remove unused func
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 4, 2020
* [TVMC] add cl support in tvmc runner

* [TVMC] use target_host when it is set

* Cleanup comment and asssert device type in else case

* add a test for tvmc compiler

* remove unused func
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Dec 4, 2020
* [TVMC] add cl support in tvmc runner

* [TVMC] use target_host when it is set

* Cleanup comment and asssert device type in else case

* add a test for tvmc compiler

* remove unused func
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants