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] [AST] Add virtual_device as a first class field in Relay #9641

Merged
merged 5 commits into from
Dec 10, 2021

Conversation

electriclilies
Copy link
Contributor

This PR adds virtual_device (SEScope) as a first class field in Relay. This is an implementation of apache/tvm-rfcs#45 please take a look the RFC for the motivation, implementation, future work, etc.

@mbs-octoml @jroesch @mikepapadim

@electriclilies electriclilies changed the title Add virtual_device as a first class field in Relay [RELAY] [AST] Add virtual_device as a first class field in Relay Dec 2, 2021
Copy link
Contributor

@mbs-octoml mbs-octoml left a comment

Choose a reason for hiding this comment

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

Thanks Lily, this can't come soon enough as I spent yesterday tracking down more accidentally-lost on_device annotations!

include/tvm/relay/expr.h Outdated Show resolved Hide resolved
@electriclilies
Copy link
Contributor Author

@comaniac Could you take a look at this? It's my implementation of apache/tvm-rfcs#45. Thanks!

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.

Overall LGTM. Just some docstring errors.

include/tvm/ir/expr.h Outdated Show resolved Hide resolved
include/tvm/relay/expr.h Outdated Show resolved Hide resolved
include/tvm/relay/expr.h Outdated Show resolved Hide resolved
include/tvm/relay/expr.h Outdated Show resolved Hide resolved
include/tvm/relay/expr.h Show resolved Hide resolved
include/tvm/relay/expr.h Outdated Show resolved Hide resolved
include/tvm/relay/expr.h Outdated Show resolved Hide resolved
@electriclilies electriclilies force-pushed the first-class-scope branch 2 times, most recently from fa8466d to 1637380 Compare December 8, 2021 21:41
@electriclilies
Copy link
Contributor Author

electriclilies commented Dec 8, 2021

@comaniac Thanks, updated the comments and docstrings! I think this is good to go in once CI is green.

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

@tqchen tqchen merged commit bd361b9 into apache:main Dec 10, 2021
@tqchen
Copy link
Member

tqchen commented Dec 10, 2021

THanks @electriclilies for the contribution. Thanks @comaniac @mbs-octoml for reviewing. This PR is now merged

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