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

[RUNTIME] Enable NDArray type extension #2598

Merged
merged 1 commit into from
Feb 14, 2019
Merged

Conversation

tqchen
Copy link
Member

@tqchen tqchen commented Feb 14, 2019

This PR proposed an improved version of tvm::NDArray signature that opens the path for future type extensions.

While the current NDArray works great, there is a potential need to extend the NDArray itself to store additional information. This can become useful when we need to introduce variants that might be scheduled by an additional async engine, or a different form of lazy/eager execution.

In these cases, we could potentially subclass the NDArray and NDArray::Container, however, this also brings a potential need for runtime type checking. This PR adds a field array_type_index_ to NDArray::Container to make the structure future compatible with the possibility of extensions. On 64 bit platform, it won't use additional space because we will need an additional int field anyway due to memory alignment issues.

@tqchen
Copy link
Member Author

tqchen commented Feb 14, 2019

@junrushao1994 @were @yzhliu @ZihengJiang please review.

@junrushao
Copy link
Member

Ok, I see your point. By checking array_type_index_, it is possible for a front end language to understand which subclass the NDArray represents.

@tqchen tqchen merged commit 6b0157b into apache:master Feb 14, 2019
@junrushao
Copy link
Member

junrushao commented Feb 15, 2019

Quick naive questions @tqchen

If an external library wants to inherit tvm arrays, and we don't want to modify TVM's codebase

  1. Does it inherit tvm::runtime::NDArray, or tvm::runtime::NDArray::Container, or both? (Looks like it is enough to inherit the container, since NDArray is just a thin layer of the container?)
  2. In which part of a frontend should the ext lib inquiry the flag array_type_index_?

@were
Copy link
Contributor

were commented Feb 15, 2019

@junrushao1994 I agree with 1 that inheriting the container only will suffice.

If inherit both, maybe they share some attributes, which should be maintained on both sides. This is not good for further maintenance.

If something like an extern::NDArray is desired, a tvm::NDArray can be created and referred by an extern::Container.

The only counter-intuitive part of doing this is that: when garbage collection, tvm::NDArray and tvm::Container free its space when ref_count_ == 1; however, for the extern::NDArray, it should be freed when ref_count_ == 2; at this point, we are going to free the last extern::Container and after that the origin tvm::NDArray can be freed.

@junrushao
Copy link
Member

extern::NDArray will be a subclass of tvm::NDArray. extern::NDArray::Container will be a subclass of tvm::NDArray::Container.

I don't understand the ref_count thing. Doesn't seem to be right...

@junrushao
Copy link
Member

From the C++ side, if we want TVMArgValue to be able to automatically convert to extern::NDArray, the TVMArgValue probably needs access to array_type_index_.

@junrushao
Copy link
Member

Probably we need a method like As<Type>(), in which the tvm::NDArray checks whether Type::array_type_index matches its data_->array_type_index_.

@were
Copy link
Contributor

were commented Feb 16, 2019

Here is the situation we are facing:
PackedFunc can only pass TVMArgs-family arguments, like tvm::NDArray.
Thus, an external call will also only get a tvm::NDArray back.
In tvm::NDArray the Container *data_ is protected, so we cannot get data_->array_type_index_.

@were
Copy link
Contributor

were commented Feb 16, 2019

To resolve this issue, it will be desirable to have a trait to force users to define a static indicator for each subclass of NDArray and Container, and provide an as<Type>() method to cast it to its corresponding subclass.

libing4752 pushed a commit to libing4752/tvm that referenced this pull request Feb 18, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
@tqchen tqchen deleted the ndarray branch February 21, 2019 00:13
@yzhliu yzhliu mentioned this pull request Mar 2, 2019
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants