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.

Sign up for GitHub

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

Jump to bottom

Support export ADT value in Python #3299

Merged
merged 4 commits into from
Jun 13, 2019
Merged

Support export ADT value in Python #3299

merged 4 commits into from
Jun 13, 2019

Conversation

wweic
Copy link
Contributor

@wweic wweic commented Jun 6, 2019 •
edited
Loading

  1. Fix prelude compilation in VM. We need to optimize functions in prelude the same as entry func.
  2. Fix ConstructorValueNode's tag and add optional constructor tag.

cc: @jroesch @MarisaKirisame @icemelon9 @zhiics @slyubomirsky @tqchen

@slyubomirsky
Copy link
Contributor

slyubomirsky commented Jun 6, 2019 •
edited
Loading

What will be the mechanism for assigning a tag to a constructor? (Forgive me if I've missed it)

@wweic
Copy link
Contributor Author

wweic commented Jun 7, 2019 •
edited
Loading

What will be the mechanism for assigning a tag to a constructor? (Forgive me if I've missed it)

@slyubomirsky a brute force approach would be to search through all constructors and compare tag to implement tag -> constructor mapping. Right now there is no data structure specifically for this.

I added a new function in prelude to map tag to constructor.

MarisaKirisame
MarisaKirisame reviewed Jun 7, 2019
View reviewed changes
src/relay/backend/interpreter.cc Outdated Show resolved Hide resolved
src/relay/backend/vm/compiler.cc Outdated Show resolved Hide resolved
zhiics
zhiics approved these changes Jun 8, 2019
View reviewed changes
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

icemelon
icemelon approved these changes Jun 9, 2019
View reviewed changes
Copy link
Member

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

LGTM

@jroesch
Copy link
Member

jroesch commented Jun 11, 2019

I'm not sure if we should use a global constructor mapping upon further thought. Many implementations of tags only number the constructors for a given type. I think it might make more sense to add an optional debugging field if we want to track the constructors.

@wweic
Copy link
Contributor Author

wweic commented Jun 11, 2019

@jroesch agree it's better to have local tag scope for each type. I'll change it back. Do you mean add debug constructor field to ContrustorValueNode?

@wweic wweic force-pushed the adt-value branch 2 times, most recently from a25ed43 to 878da97 Compare June 11, 2019 02:01
@wweic
Copy link
Contributor Author

wweic commented Jun 11, 2019

@jroesch @slyubomirsky Addressed comments. Please take another look. Thanks.

MarisaKirisame
MarisaKirisame reviewed Jun 11, 2019
View reviewed changes
src/relay/backend/interpreter.cc Outdated Show resolved Hide resolved
src/relay/backend/vm/compiler.cc Outdated Show resolved Hide resolved
@zhiics zhiics mentioned this pull request Jun 12, 2019
Closed
@slyubomirsky
Copy link
Contributor

LGTM, thanks

@apivovarov
Copy link
Contributor

@slyubomirsky Can you approve it in addition to saying LGTM?

@MarisaKirisame @icemelon9 @zhiics @slyubomirsky Can it be merged ASAP? I'm constantly getting test_sum_loop Segmentation fault errors #3352

@jroesch jroesch merged commit 713fc73 into apache:master Jun 13, 2019
This was referenced Jun 13, 2019
Merged
Merged
wweic added a commit to wweic/tvm that referenced this pull request Jun 26, 2019
@wweic
Support export ADT value in Python (apache#3299)
9ab290f 
* Support export ADT value in Python

* Cache original functions

* Cleanup

* Cleanup
wweic added a commit to neo-ai/tvm that referenced this pull request Jun 27, 2019
@wweic
Support export ADT value in Python (apache#3299)
27ffd50 
* Support export ADT value in Python

* Cache original functions

* Cleanup

* Cleanup
@wweic wweic deleted the adt-value branch July 18, 2019 19:57
@tqchen tqchen mentioned this pull request Nov 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slyubomirsky slyubomirsky slyubomirsky left review comments

@icemelon icemelon icemelon approved these changes

@zhiics zhiics zhiics approved these changes

@MarisaKirisame MarisaKirisame MarisaKirisame approved these changes

@jroesch jroesch jroesch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@wweic @slyubomirsky @jroesch @apivovarov @icemelon @MarisaKirisame @zhiics