[VM][Textures] Enable OpenCL textures for VM #15419
Conversation
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
echuraev
force-pushed
the
echuraev/enable_textures_for_vm
branch
2 times, most recently
from
913e239 to
e421068
Compare
|
@masahi, @csullivan, @elvin-n, @srkreddy1238 CI is green. Could you please review this PR? |
This commit introduces memory scope to VM and enables using textures.
The following changes have been made:
- AnnotateMemoryScope pass is used in VM compilation pipeline
- VM allows to use more than one device with the same device type.
Also, virtual devices in VM contains information about memory
scope.
- Instructions LoadConst and AllocStorage were extended to support
textures.
- VM bytecode was updated to support memory scope.
- Annotate texture storage pass was updated to support dynamic shape.
- Some other minor changes have been made.
echuraev
force-pushed
the
echuraev/enable_textures_for_vm
branch
from
884dad1 to
47918ea
Compare
| auto mem_scope = exec_->virtual_devices[instr.device_copy.dst_device_index].second; | ||
|
|
||
| NDArray dst_data = src_data.CopyTo(dst_dev); | ||
| NDArray dst_data = src_data.CopyTo(dst_dev, String(mem_scope)); |
There was a problem hiding this comment.
This is creating a string copy. Possible to avoid it?
There was a problem hiding this comment.
I tried to do that, but it looks like we cannot avoid it. We cannot store mem_scope in virtual_devices as a String because of serialization problems. We could use std::string in CopyTo method, but it looks like in NDArray it is better to use String because of keeping uniform interface.
There was a problem hiding this comment.
Does the instr.device_copy.dst NDArray not have the correct memory scope already?
-
If so, then can we use it instead of the copy here?
-
If not, then I would not expect
instr.device_copy.srcto have the correct mem_scope either. In that case you would need to construct a new ndarray from a container using the src dataptr and the correct memory scope.
There was a problem hiding this comment.
Sorry, @csullivan, probably I didn't get your question. So, I'll try to reply in details and please, correct me if I won't answer on your questions.
device_copy instruction contains three fields: src register, src device index and dst device index. So I extract mem_scope for dst memory from dst device index and it gets me the correct memory scope for destination array.
Answering on the first question: If you speak about copy data to dst NDArray, then no, we have to do such copy because dst NDArray will be created after this copy. If you speak about creating a string copy, then I have answered on this question in the previous message.
Answering on the second question: I extract destination memory scope from inst.device_copy.dst_device_index, so extracted memory scope is correct. And I create a new NDArray from a src dataptr with correct memory scope.
echuraev
force-pushed
the
echuraev/enable_textures_for_vm
branch
from
b82e1fb to
67a22d9
Compare
| case Opcode::KillRegister: | ||
| return; | ||
| case Opcode::AllocStorage: | ||
| if (this->alloc_storage.ndim > 0) { |
There was a problem hiding this comment.
I'd prefer more direct
this->alloc_storage.shape != nullptr
since this->alloc_storage.ndim > 0 is assuming the logic implemented elsewhere that the shape is non-null only when this->alloc_storage.ndim > 0.
There was a problem hiding this comment.
I'm afraid that check on nullptr might be not enough, because shape and allocation_size are in the same union. When we change the value of allocation_size then shape is not equal to nullptr and when we try to release "allocated" memory in shape then the program might be crashed with segfault.
With current implementation of alloc_storage instruction, we allocate memory for shape only in case when ndim > 0 so this is why I think that it is a valid condition for releasing memory.
If you have any ideas how to implement this logic more clearly, then let me know.
4 participants
This commit introduces memory scope to VM and enables using textures.
The following changes have been made:
Also, virtual devices in VM contains information about memory
scope.
textures.
cc: @masahi, @csullivan, @elvin-n, @srkreddy1238