You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/09/29 02:43:25 UTC

[GitHub] [incubator-tvm] tqchen opened a new pull request #6586: [RUNTIME] NDArray CopyFrom/To Bytes always synchronize

tqchen opened a new pull request #6586:
URL: https://github.com/apache/incubator-tvm/pull/6586


   The previous behavior of non-sync can be unsafe for GPU devices.
   In particular, the need for an explicit synchronization could
   leads to confusion behavior e.g. asnumpy does not immediately return
   the right content for vulkan.
   
   Also brings the requirement of array being contiguous.
   Right now we encourage compact array since they are easier for optimization.
   We can consider bring support later by introducing a compactify PackedFunc(which might need be jitted).
   
   Thanks for contributing to TVM!   Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-tvm] tqchen commented on pull request #6586: [RUNTIME] NDArray CopyFrom/To Bytes always synchronize

Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6586:
URL: https://github.com/apache/incubator-tvm/pull/6586#issuecomment-700392126


   cc @tmoreau89 @ZihengJiang @zhiics @tkonolige 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6586: [RUNTIME] NDArray CopyFrom/To Bytes always synchronize

Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6586:
URL: https://github.com/apache/incubator-tvm/pull/6586#discussion_r496361808



##########
File path: src/runtime/ndarray.cc
##########
@@ -70,9 +70,12 @@ void ArrayCopyFromBytes(DLTensor* handle, const void* data, size_t nbytes) {
   cpu_ctx.device_id = 0;
   size_t arr_size = GetDataSize(*handle);
   CHECK_EQ(arr_size, nbytes) << "ArrayCopyFromBytes: size mismatch";
+  CHECK(IsContiguous(*handle)) << "ArrayCopyFromBytes only support contiguous array for now";
   DeviceAPI::Get(handle->ctx)
       ->CopyDataFromTo(data, 0, handle->data, static_cast<size_t>(handle->byte_offset), nbytes,
                        cpu_ctx, handle->ctx, handle->dtype, nullptr);
+  // Synchronize in case data become inavailable later.

Review comment:
       ```suggestion
     // Synchronize in case data become unavailable later.
   ```

##########
File path: src/runtime/ndarray.cc
##########
@@ -81,9 +84,12 @@ void ArrayCopyToBytes(const DLTensor* handle, void* data, size_t nbytes) {
   cpu_ctx.device_id = 0;
   size_t arr_size = GetDataSize(*handle);
   CHECK_EQ(arr_size, nbytes) << "ArrayCopyToBytes: size mismatch";
+  CHECK(IsContiguous(*handle)) << "ArrayCopyToBytes only support contiguous array for now";
   DeviceAPI::Get(handle->ctx)
       ->CopyDataFromTo(handle->data, static_cast<size_t>(handle->byte_offset), data, 0, nbytes,
                        handle->ctx, cpu_ctx, handle->dtype, nullptr);
+  // Synchronize in case data become inavailable later.

Review comment:
       ```suggestion
     // Synchronize in case data become unavailable later.
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-tvm] ANSHUMAN87 commented on a change in pull request #6586: [RUNTIME] NDArray CopyFrom/To Bytes always synchronize

Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on a change in pull request #6586:
URL: https://github.com/apache/incubator-tvm/pull/6586#discussion_r496975400



##########
File path: src/runtime/ndarray.cc
##########
@@ -70,9 +70,12 @@ void ArrayCopyFromBytes(DLTensor* handle, const void* data, size_t nbytes) {
   cpu_ctx.device_id = 0;
   size_t arr_size = GetDataSize(*handle);
   CHECK_EQ(arr_size, nbytes) << "ArrayCopyFromBytes: size mismatch";
+  CHECK(IsContiguous(*handle)) << "ArrayCopyFromBytes only support contiguous array for now";

Review comment:
       I think we can add this check earliest!

##########
File path: src/runtime/ndarray.cc
##########
@@ -81,9 +84,12 @@ void ArrayCopyToBytes(const DLTensor* handle, void* data, size_t nbytes) {
   cpu_ctx.device_id = 0;
   size_t arr_size = GetDataSize(*handle);
   CHECK_EQ(arr_size, nbytes) << "ArrayCopyToBytes: size mismatch";
+  CHECK(IsContiguous(*handle)) << "ArrayCopyToBytes only support contiguous array for now";

Review comment:
       same as above!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-tvm] tqchen commented on pull request #6586: [RUNTIME] NDArray CopyFrom/To Bytes always synchronize

Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6586:
URL: https://github.com/apache/incubator-tvm/pull/6586#issuecomment-700974264


   Thanks @tmoreau89 @zhiics @ANSHUMAN87 @ZihengJiang 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-tvm] tqchen merged pull request #6586: [RUNTIME] NDArray CopyFrom/To Bytes always synchronize

Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #6586:
URL: https://github.com/apache/incubator-tvm/pull/6586


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-tvm] tqchen commented on pull request #6586: [RUNTIME] NDArray CopyFrom/To Bytes always synchronize

Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6586:
URL: https://github.com/apache/incubator-tvm/pull/6586#issuecomment-700772795


   Note that the async behavior can still be fully recovered by Allocating a CPU NDArray and copy from there. The current behavior only affects copyfromto bytes


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6586: [RUNTIME] NDArray CopyFrom/To Bytes always synchronize

Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6586:
URL: https://github.com/apache/incubator-tvm/pull/6586#discussion_r497034108



##########
File path: src/runtime/ndarray.cc
##########
@@ -70,9 +70,12 @@ void ArrayCopyFromBytes(DLTensor* handle, const void* data, size_t nbytes) {
   cpu_ctx.device_id = 0;
   size_t arr_size = GetDataSize(*handle);
   CHECK_EQ(arr_size, nbytes) << "ArrayCopyFromBytes: size mismatch";
+  CHECK(IsContiguous(*handle)) << "ArrayCopyFromBytes only support contiguous array for now";

Review comment:
       Thanks @ANSHUMAN87 in this case i think the order of the check does not matter as much.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org