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

[Refactor][Relay Build] refactor build module to take IRModule #4988

Merged
merged 1 commit into from
Mar 5, 2020

Conversation

zhiics
Copy link
Member

@zhiics zhiics commented Mar 5, 2020

This PR refactors relay build_module interfaces to accept IRModules instead of Function. This can help external codegen that may need to lift functions into the global scope.

Many tests in the codebase have already passed IRModule to relay.build, so no additional unit test is added in this PR.

A follow up PR will be sent to add the outlining and inlining of subgraphs/sub-functions that are supported by external codegen.

cc @comaniac @icemelon9 @jroesch @tqchen @wweic

@zhiics zhiics force-pushed the build_mod branch 2 times, most recently from 5d662d1 to 8376093 Compare March 5, 2020 00:24
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. Just a miner point: We might consider accepting the Function for a while like build if we expect users to call BuildModule.build and BuildModule.optimize directly. Otherwise, please ignore this comment.

@zhiics
Copy link
Member Author

zhiics commented Mar 5, 2020

Yeah, Python side still accepts Function as well. Its just a line of code tvm::IRModule::FromExpr(func); in c++, so I don't want to overload Build.

@comaniac
Copy link
Contributor

comaniac commented Mar 5, 2020

I see. That makes sense.

@wweic
Copy link
Contributor

wweic commented Mar 5, 2020

LGTM. Should we also update the tutorials?

@zhiics
Copy link
Member Author

zhiics commented Mar 5, 2020

@wweic Its okay. Tutorials have already used module.

@zhiics zhiics merged commit f63b249 into apache:master Mar 5, 2020
@zhiics zhiics deleted the build_mod branch March 5, 2020 15:37
@zhiics
Copy link
Member Author

zhiics commented Mar 5, 2020

Thanks @comaniac @tqchen @wweic

@tqchen
Copy link
Member

tqchen commented Mar 5, 2020

@zhiics seems this change breaks the master ci, please look into it

@zhiics
Copy link
Member Author

zhiics commented Mar 5, 2020

Okay. I will. Thanks for reminding.

@zhiics
Copy link
Member Author

zhiics commented Mar 5, 2020

hmm, this actually revealed that the gcn tutorial didn't pass mod. We need to bind params when adding the function to the mod. Interestingly, it didn't fail for the last two runs before merging. #4994

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Apr 16, 2020
zhiics added a commit to neo-ai/tvm that referenced this pull request Apr 17, 2020
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