[VM] check DLManagedTensor for conditions to construct NDArray #11504
Conversation
vvchernov
force-pushed
the
vc/dltensor_conditions_in_vm
branch
from
2d0b447 to
f0da140
Compare
src/runtime/ndarray.cc
Outdated
| // fill up content. | ||
| data->manager_ctx = tensor; | ||
| ICHECK(::tvm::runtime::IsContiguous(tensor->dl_tensor)) | ||
| << "DLManagedTensor is not contiguous. It does not support for now"; |
There was a problem hiding this comment.
| << "DLManagedTensor is not contiguous. It does not support for now"; | |
| << "DLManagedTensor must be contiguous."; |
There was a problem hiding this comment.
Ok, I will redo it. But I based on similar description in other ICHECK. I think TVM design assumes contiguous tensors only and what we can potentially support in the future it is copying from incontiguous to contiguous ones.
There was a problem hiding this comment.
Personally I think saying something will be supported in the future doesn't really add any useful information. But, if you do want to say that "Only contiguous DLManagedTensors are currently supported." would be a better message.
There was a problem hiding this comment.
How about "DLTensor is not contiguous. Copying from non-contiguous data is currently not supported."
There was a problem hiding this comment.
Ok, done. It still is not clear for me descriptions from 65 and 83 lines (initial code). Looks like idea is related to non-contiguous data copying, but I'm not sure
There was a problem hiding this comment.
The wording on those lines is a little awkward but I believe they mean that ArrayCopyFromBytes and ArrayCopyToBytes only support contiguous data.
vvchernov
force-pushed
the
vc/dltensor_conditions_in_vm
branch
from
760512b to
2e69b94
Compare
|
Hello @tkonolige could you approve it? if not we can ask Thierry |
|
I've approved it. But you'll also need a committer to approve it. @tmoreau89 |
tmoreau89
left a comment
tmoreau89
left a comment
There was a problem hiding this comment.
Thanks @tkonolige for the review, LGTM
|
Thanks @vvchernov and @tkonolige - the PR has been merged. |
3 participants
During developing #11003 #11391 It was observed that DLTensor should satisfy some conditions (data contiguous and alignment) for correct construction of NDArray based on it to avoid problems with memory using.
Further analysis of NDArray class methods showed the same problem in other public method. There is fix of it.
@tkonolige could you check it?