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

[Relay] Invoke tvm::build from relay compile_engine and interpreter #4723

Merged
merged 1 commit into from
Jan 17, 2020

Conversation

hlu1
Copy link
Contributor

@hlu1 hlu1 commented Jan 16, 2020

@hlu1 hlu1 requested a review from tqchen January 16, 2020 02:53
Copy link
Contributor

@MarisaKirisame MarisaKirisame left a comment

Choose a reason for hiding this comment

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

Why isnt this a line by line translation?

@hlu1 hlu1 force-pushed the relay_backend_build branch 2 times, most recently from ddf0ec9 to ef0acee Compare January 16, 2020 07:17
@hlu1
Copy link
Contributor Author

hlu1 commented Jan 16, 2020

Sorry I missed a few things at the beginning and just added them.

Do you mean translating this part line by line, https://github.com/apache/incubator-tvm/blob/master/python/tvm/build_module.py#L509-L650?

Some of the type checks are not needed in C++, and the signature of https://github.com/apache/incubator-tvm/blob/bfb4884e47f7d2c759b2ee707aa18acf35116380/python/tvm/relay/backend/_backend.py#L65-L88 is a lot simpler.

Also,
https://github.com/apache/incubator-tvm/blob/master/python/tvm/build_module.py#L635-L650 has been implemented in here https://github.com/apache/incubator-tvm/blob/master/src/codegen/build_module.cc#L278-L319. All I need to do is call this function.

@hlu1 hlu1 force-pushed the relay_backend_build branch from ef0acee to b889a8f Compare January 16, 2020 07:34
@MarisaKirisame
Copy link
Contributor

I know very little about tvm Module and relay build, so I might get something wrong.
But it seems like you replace 27 line with 47 line, but they are implemented completely differently - one call another function while another does lots of work. Any reason for this?

@tqchen
Copy link
Member

tqchen commented Jan 16, 2020

cc @zhiics @jroesch please also help to take a look

@zhiics
Copy link
Member

zhiics commented Jan 16, 2020

@hlu1 Can we simply remove this packed function and directly invoke tvm::build from compile_engine and interpreter? We have two separate pipelines from Python and C++. I think we can probably directly use tvm.build from python and tvm::build from C++. So we don't really need to interoperate between them.

@hlu1
Copy link
Contributor Author

hlu1 commented Jan 16, 2020

@zhiics, makes sense. Will do.

@hlu1 hlu1 force-pushed the relay_backend_build branch from b889a8f to 226d2d1 Compare January 17, 2020 01:38
@hlu1 hlu1 changed the title [Relay] Port relay.backend.build to c++ [Relay] Invoke tvm::build from relay compile_engine and interpreter Jan 17, 2020
} else {
LOG(FATAL) << "relay.backend.build is not registered";
m = build(value->cached_func->funcs, key->target, key->target, BuildConfig::Current());
Copy link
Member

Choose a reason for hiding this comment

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

according to the syntax we had in Python, should we do the following?

m = build(value->cached_func->funcs, key->target, Target(nullptr), BuildConfig::Current());

Same to the one in intepreter.cc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@hlu1 hlu1 force-pushed the relay_backend_build branch from 226d2d1 to ba4ed82 Compare January 17, 2020 07:00
Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

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

LGTM

@zhiics zhiics merged commit 3279957 into apache:master Jan 17, 2020
@zhiics
Copy link
Member

zhiics commented Jan 17, 2020

Thanks @hlu1 @MarisaKirisame

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.

4 participants