Added CopyFromBytes and CopyToBytes convenience methods to NDArray. Fixed typos. #4970
Conversation
jmorrill
changed the title
include/tvm/runtime/ndarray.h
Outdated
| @@ -306,6 +324,26 @@ inline void NDArray::CopyFrom(const NDArray& other) { | |||
| CopyFromTo(&(other.get_mutable()->dl_tensor), &(get_mutable()->dl_tensor)); | |||
| } | |||
|
|
|||
| inline void NDArray::CopyFromBytes(const void* data, size_t nbytes) { | |||
There was a problem hiding this comment.
To avoid duplication, let us make it a non-line member function, and make uses of implementation in the C API here https://github.com/apache/incubator-tvm/blob/master/src/runtime/ndarray.cc#L289
After that, we can safely redirect the C API calls into these C++ APIs
There was a problem hiding this comment.
Ok, I have it calling the TVMArrayCopyFrom/ToBytes methods.
Are you saying to put the implementation into ndarray.cc ?
There was a problem hiding this comment.
Ok, I misunderstood your original message. Let me fix
include/tvm/runtime/ndarray.h
Outdated
| * \note The copy may happen asynchronously if it involves a GPU context. | ||
| * TVMSynchronize is necessary. | ||
| */ | ||
| inline void CopyFromBytes(const void* data, size_t nbytes); |
There was a problem hiding this comment.
Declare as TVM_DLL as it is a member function that is not inlined
src/runtime/ndarray.cc
Outdated
| void NDArray::CopyToBytes(void* data, size_t nbytes) const { | ||
| CHECK(data != nullptr); | ||
| CHECK(data_ != nullptr); | ||
| CHECK_EQ(TVMArrayCopyToBytes(&get_mutable()->dl_tensor, data, nbytes), 0); |
There was a problem hiding this comment.
We should move the impl of TVMArrayCopyToBytes to here, and call this function from that function instead. So exceptions can be directly throw out without in C++
There was a problem hiding this comment.
Not a problem.. Sorry it has taken so many revisions :). At least they are easy changes.
There was a problem hiding this comment.
no worries, we always need to iterate to make better versions. Thanks for being patient in improving things
| @@ -185,6 +185,38 @@ NDArray NDArray::FromDLPack(DLManagedTensor* tensor) { | |||
| return NDArray(GetObjectPtr<Object>(data)); | |||
| } | |||
|
|
|||
| void NDArray::CopyToBytes(void* data, size_t nbytes) const { | |||
| CHECK(data != nullptr); | |||
There was a problem hiding this comment.
Last change: perhaps it is better to bring the common impl in this function, and TVMArrayCopyToBytes to a common function
void ArrayCopyToBytes(DLTensor* handle, void* data, size_t nbytes).
and have both functions call into this common function to avoid impl duplications.
There was a problem hiding this comment.
I have ArrayCopyFromBytes/ArrayCopyToBytes as the single implementation for the C and C++ API.
Let me know if I missed anything.
|
Thanks @jmorrill !! |
tqchen
added
status: accepted
and removed
status: need update
…ixed typos. (apache#4970) * Added CopyFromBytes and CopyToBytes convenience methods. Fixed typos. * Removed unneed argument check * Use TVMArrayCopyFrom/ToBytes methods * Moved CopyFrom/ToBytes to ndarray.cc * CopyToBytes impl was using CopyFromBytes. Fixed * changed inline to TVM_DLL * Used impl from TVMArrayCopyTo/FromBytes into NDArray CopyTo/FromBytes * Move implementation of all CopyFrom/ToBytes into a common impls * make arg const * simplify method impl
…ixed typos. (apache#4970) * Added CopyFromBytes and CopyToBytes convenience methods. Fixed typos. * Removed unneed argument check * Use TVMArrayCopyFrom/ToBytes methods * Moved CopyFrom/ToBytes to ndarray.cc * CopyToBytes impl was using CopyFromBytes. Fixed * changed inline to TVM_DLL * Used impl from TVMArrayCopyTo/FromBytes into NDArray CopyTo/FromBytes * Move implementation of all CopyFrom/ToBytes into a common impls * make arg const * simplify method impl
It is not very intuitive (specially as a beginner) on how to copy a
void*to anNDArray. Currently one may have to configure aDLTensorand run anNDArray::CopyTo(...).This PR adds two member functions to NDArray to copy to and from a
void*.NDArray::CopyFromBytes(const void* data, size_t nbytes)and
NDArray::CopyToBytes(void* data, size_t nbytes)Included in this PR are also a few typo fixes.