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 Object in visit API #54

Merged
merged 5 commits into from
Mar 26, 2019
Merged

Support Object in visit API #54

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
154a905
Support VMObject in visit API
jroesch Feb 5, 2019
cddd070
Rename VMObject
jroesch Mar 26, 2019
efb14e3
Fix header
jroesch Mar 26, 2019
b133e55
Move into runtime namespace
jroesch Mar 26, 2019
cea4958
Use class
jroesch Mar 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/tvm/node/node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ namespace runtime {
class NDArray;
} // namespace runtime

namespace vm {
Copy link
Member

@tqchen tqchen Mar 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

perhaps make it runtime::vm::VMObject? The runtime namespace is isolated and can be compiled separately(in AOT)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was planning to rename right now, to simply the organization, just pushed a commit.

Copy link
Member

Choose a reason for hiding this comment

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

is VMObject a reference class(like NDArray) or the container itself? Ideally we want it to be the reference class

Copy link
Member

Choose a reason for hiding this comment

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

could likely be merged into the namspace above(runtime::NDArray)

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a reference class, we can move into runtime if you want

Copy link
Member

Choose a reason for hiding this comment

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

You can make the decison. Perhaps it makes sense to move to runtime given that there is no other runtime Object

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, not sure if we should keep it isolated or not. Maybe it makes sense if we will reuse for AoT/JiT, etc.

// forward declaration
struct Object;
} // namespace runtime

/*!
* \brief Visitor class to each node content.
* The content is going to be called for each field.
Expand All @@ -41,6 +46,7 @@ class EXPORT AttrVisitor {
virtual void Visit(const char* key, Type* value) = 0;
virtual void Visit(const char* key, NodeRef* value) = 0;
virtual void Visit(const char* key, runtime::NDArray* value) = 0;
virtual void Visit(const char* key, runtime::vm::Object* value) = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Let us just do runtime::Object then

Copy link
Member Author

Choose a reason for hiding this comment

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

I just decided right before you posted this ha. Just pushed a commit to do so.

template<typename ENum,
typename = typename std::enable_if<std::is_enum<ENum>::value>::type>
void Visit(const char* key, ENum* ptr) {
Expand Down