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][Runtime] Add support for virtual machine Objects #3120

Merged
merged 10 commits into from
May 2, 2019

Conversation

jroesch
Copy link
Member

@jroesch jroesch commented Apr 30, 2019

This PR separates the functionality for passing runtime::Objects around from the VM, VM memory manager and VM compiler.

See #2810 and #2889.

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.

src/runtime/object.cc Outdated Show resolved Hide resolved
@jroesch jroesch changed the title Add support for virtual machine Objects [Relay][Runtime] Add support for virtual machine Objects Apr 30, 2019
include/tvm/runtime/object.h Outdated Show resolved Hide resolved
*/
class Object {
public:
ObjectPtr<ObjectCell> ptr;
Copy link
Member

Choose a reason for hiding this comment

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

because we do not want the user to touch ptr, consider rename to ptr_,

src/runtime/object.cc Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented May 1, 2019

Some minor nits. After some thoughts, I personally do not like the fact that Node and Object system are separate. Eventually, we want to merge them into a single node system, but still have a clear separation of compiler and runtime node objects.

For now, we could even reuse the NodeBase class for object(which allows the effective use of make_node). This may need to happen in a few months and I don't want to block the PR, but it might be helpful to keep that in mind when polishing this PR, e.g., the naming(XXXCell-> XXXNode?).

Possible way of unification, for reference:

  • NodeBase contains a tag field
  • ASTNode subclass NodeBase, and has additional virtual functions for AST level support(attr travseral etc), the tags can dynamically allocated during runtime.
  • Object is a subclass of NodeBase, with fixed tags.

@tqchen tqchen self-assigned this May 1, 2019
Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

LGTM: unaddress comment: Please rename Object::ptr ->Object::ptr_

@tqchen tqchen merged commit dd55682 into apache:master May 2, 2019
@tqchen
Copy link
Member

tqchen commented May 2, 2019

Thanks, @jroesch @wweic @weberlo this 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