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 C++ Build Module #3082

Merged
merged 2 commits into from
May 8, 2019
Merged

Relay C++ Build Module #3082

merged 2 commits into from
May 8, 2019

Conversation

antinucleon
Copy link
Contributor

@antinucleon antinucleon commented Apr 23, 2019

Currently most schedules are written in Python. I tested on CPU environment this building module is able to pass a dense + relu test with Python schedules. To fully replace Python build module, port some AutoTVM feature to C++ is necessary.

@antinucleon antinucleon changed the title [WIP] Relay C++ Build Module Relay C++ Build Module Apr 23, 2019
@antinucleon
Copy link
Contributor Author

ping reviewers...

include/tvm/build_module.h Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
include/tvm/build_module.h Outdated Show resolved Hide resolved
@jroesch
Copy link
Member

jroesch commented Apr 27, 2019

Sorry about the slow review, been a busy week! Thanks for rewriting so much code into C++, my feedback is mostly stylistic.

Can you review @MarisaKirisame?

1 similar comment
@jroesch
Copy link
Member

jroesch commented Apr 27, 2019

Sorry about the slow review, been a busy week! Thanks for rewriting so much code into C++, my feedback is mostly stylistic.

Can you review @MarisaKirisame?

@tqchen tqchen mentioned this pull request Apr 27, 2019
5 tasks
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
tests/python/relay/test_cpp_build_module.py Outdated Show resolved Hide resolved
@alex-weaver
Copy link
Contributor

General feedback: I would suggest a pass over the code to revise the \brief comments - try to include the purpose of the class/function.

include/tvm/build_module.h Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
@jroesch jroesch self-assigned this May 1, 2019
@jroesch jroesch added the status: need update need update based on feedbacks label May 1, 2019
@antinucleon antinucleon force-pushed the build branch 9 times, most recently from fe80f40 to cbdd452 Compare May 6, 2019 23:52
@antinucleon
Copy link
Contributor Author

@jroesch Updated.

@antinucleon antinucleon force-pushed the build branch 3 times, most recently from c7fc5e7 to 05c828d Compare May 7, 2019 07:14
@icemelon
Copy link
Member

icemelon commented May 7, 2019

@jroesch @MarisaKirisame @tqchen Could one of you review this PR again?

@antinucleon
Copy link
Contributor Author

Updated.

src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
@antinucleon antinucleon force-pushed the build branch 2 times, most recently from fdb9f8d to 532cbd3 Compare May 7, 2019 21:27
@jroesch
Copy link
Member

jroesch commented May 7, 2019

@antinucleon Looks good to me, one last question when will we replace the Python code currently in build_module.py? having two code paths that do the same thing makes me nervous, it would be good to remove all that code if possible.

Copy link
Contributor

@wweic wweic left a comment

Choose a reason for hiding this comment

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

lgtm

@antinucleon
Copy link
Contributor Author

@jroesch We have to wait AutoTVM is ported to C++, then we are able to replace Python build_module.

@jroesch
Copy link
Member

jroesch commented May 7, 2019

Why can't we just expose the necessary Python functions and call them from C++?

src/codegen/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
std::unordered_set<std::string> disabled_pass;
OptPassLevel OPT_PASS_LEVEL;
inline bool pass_enabled(const std::string& pass_name) const {
if (disabled_pass.count(pass_name)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

CHECK_GT(add_pass.count(pass_name), 0);
inside this if.
This will catch corner case where a pass_name is both enabled and disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disabled pass priority is higher than adding extra pass: see https://discuss.tvm.ai/t/relay-alter-op-layout-pass-regression/1870/2. I don't know any use case which people will enable some extra passes and disable at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, if there is no such use case, better just do not allow it, so in case ppl enable and disable by accident, there will be an immediate error.

Copy link
Contributor Author

@antinucleon antinucleon May 8, 2019

Choose a reason for hiding this comment

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

My point is: There is no use case to both enable and disable a pass, but we may need O3 but disable some passes.

src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
@antinucleon
Copy link
Contributor Author

@jroesch For fully migrating to C++, we have to do step by step, as the python module is not typed and fixing all these case need quite a lot of work.

@jroesch
Copy link
Member

jroesch commented May 8, 2019

Ok sounds good. We can do it incrementally.

@jroesch jroesch merged commit b131d83 into apache:master May 8, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request May 13, 2019
* [Relay] C++ Build module

* asdf
wweic pushed a commit to neo-ai/tvm that referenced this pull request May 13, 2019
* [Relay] C++ Build module

* asdf
@tqchen tqchen mentioned this pull request May 16, 2019
@antinucleon antinucleon deleted the build branch May 22, 2019 22:01
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.

6 participants