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 2022/05/18 14:30:57 UTC
[GitHub] [tvm] vvchernov opened a new pull request, #11358: WIP: [Virtual Machine] Implementation of 'set_output_zero_copy'
vvchernov opened a new pull request, #11358:
URL: https://github.com/apache/tvm/pull/11358
This is a draft of implementation of 'set_output_zero_copy' method on VirtualMachine side.
Brief description of approach.
1. There is python API function 'set_output' which save external outputs in VM outputs_ field (map) for specified func name. It looks like 'set_input' method.
2. During 'invoke' outputs_ are saved in register_file. For this the register index of result is found from code_ field.
3. Due to we already have pre-allocated tensor(s) in register, memory allocation for the register index is avoided. Just now it has been done for AllocTensor and AllocADT ops (I observed in tests for different models that they are used for result tensors). May be other Alloc ops also should be updated.
Notes: 1. I'm not sure that it works for many frames. Practically it looks like we need code_ not frame and any number of frames does not change ops stack (code_). Other thing I observed that code_ does not depend on func_name, may be it should it is not threadsafe just now.
2. It was implemented for CPU, I plan to check GPU specifics.
3. It seems that tensor(s) is(are) allocated over storage with prepared memory. It means that skipped AllocTensor and AllocADT can keep memory in RAM, it is not good thing but it generates questions about scenarios and VM flexibility.
Hello @altanh and @mbs-octoml! Could you see the draft?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbs-octoml commented on a diff in pull request #11358: WIP: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
mbs-octoml commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r876367929
##########
src/runtime/vm/vm.cc:
##########
@@ -521,13 +603,25 @@ int64_t VirtualMachine::LoadScalarInt(Index r) const {
return result;
}
-void VirtualMachine::RunLoop() {
+Index VirtualMachine::GetResultRegisterIndex() const {
+ Index op_ind = 0;
+ Instruction instr;
+ // TODO(vvchernov): can it be endless loop?
+ do {
+ instr = code_[op_ind++];
+ } while (instr.op == Opcode::Ret);
+
+ return instr.result;
+}
+
+void VirtualMachine::RunLoop(bool set_output_enabled) {
ICHECK(this->exec_);
ICHECK(this->code_);
pc_ = 0;
Index frame_start = frames_.size();
- while (true) {
- main_loop:
+ Index res_reg_index = GetResultRegisterIndex();
Review Comment:
For tuple results we'd need to redirect the allocs for the tuple indexes, which may be scattered quite widely within the code. So that suggests the compiler should record metadata for all that.
But I'm wondering if it would be better to bite-the-bullet and switch the VM to DPS. After that only Invoke-without-outputs would be the special case, requiring inspection of metadata to alloc the output tensors, make the call, and optionally construct the tuple result. I know that's a much bigger change, and I guess we could make that change after this PR given the invoke APIs will be the same.
Is there a high pressure customer use case which justifies that two step approach?
##########
src/runtime/vm/vm.cc:
##########
@@ -537,7 +631,7 @@ void VirtualMachine::RunLoop() {
from_obj = ReadRegister(instr.from);
WriteRegister(instr.dst, from_obj);
pc_++;
- goto main_loop;
+ break;
Review Comment:
nit: This weird control flow may be the result of someone trying to improve I$ behavior so we should check. I recall there's a known issue with the VM being slightly slower than the GraphExecutor due to cache issues, but I suspect that's more likely to be due to D$ effects with the extra indirections around registers or something.
##########
src/runtime/vm/vm.cc:
##########
@@ -521,13 +603,25 @@ int64_t VirtualMachine::LoadScalarInt(Index r) const {
return result;
}
-void VirtualMachine::RunLoop() {
+Index VirtualMachine::GetResultRegisterIndex() const {
+ Index op_ind = 0;
+ Instruction instr;
+ // TODO(vvchernov): can it be endless loop?
+ do {
+ instr = code_[op_ind++];
+ } while (instr.op == Opcode::Ret);
Review Comment:
!= Opcode::Ret
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: WIP: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r890031981
##########
src/runtime/vm/vm.cc:
##########
@@ -521,13 +603,25 @@ int64_t VirtualMachine::LoadScalarInt(Index r) const {
return result;
}
-void VirtualMachine::RunLoop() {
+Index VirtualMachine::GetResultRegisterIndex() const {
+ Index op_ind = 0;
+ Instruction instr;
+ // TODO(vvchernov): can it be endless loop?
+ do {
+ instr = code_[op_ind++];
+ } while (instr.op == Opcode::Ret);
+
+ return instr.result;
+}
+
+void VirtualMachine::RunLoop(bool set_output_enabled) {
ICHECK(this->exec_);
ICHECK(this->code_);
pc_ = 0;
Index frame_start = frames_.size();
- while (true) {
- main_loop:
+ Index res_reg_index = GetResultRegisterIndex();
Review Comment:
Hello @mbs-octoml! I've updated my code for tuple results. I get register indices from AllocADT instruction and further there is the same action as for AllocTensor
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r968751586
##########
src/runtime/vm/vm.cc:
##########
@@ -143,8 +143,15 @@ PackedFunc VirtualMachine::GetFunction(const std::string& name,
} else {
auto it = inputs_.find(func_name);
ICHECK(it != inputs_.end()) << "Input has not been set for function " << func_name;
- const std::vector<ObjectRef>& func_args = it->second;
- *rv = Invoke(func, func_args);
+ const std::vector<ObjectRef>& input_args = it->second;
+ if (set_outputs_enabled_.count(func_name) && set_outputs_enabled_[func_name]) {
+ ICHECK(outputs_.count(func_name))
+ << "Outputs have not been set for function " << func_name;
+ *rv = Invoke(func, input_args, outputs_[func_name]);
+ set_outputs_enabled_[func_name] = false;
Review Comment:
I have done it due to the second scenario is valid in this case but It should be noted that the first one is not. Each invoke the register file is refilled, we need to keep outputs_ if we want to save result in the same memory. Perhaps the API for the first scenario should be thought out more deeply and implemented separately. Just now the second scenario works only.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
tkonolige commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r966291114
##########
include/tvm/runtime/vm/vm.h:
##########
@@ -339,6 +371,51 @@ class TVM_DLL VirtualMachine : public runtime::ModuleNode {
void SetInputTensorWithIndex(std::vector<ObjectRef>& tensors, // NOLINT(*)
const TVMArgValue& tensor, int index, Device dev);
+ /*!
+ * \brief Convert tensor from TVMArgValue to ObjectRef.
+ * DLTensor and NDArray types are supported.
+ * \param tensor given arg value containing tensor.
+ * \return tensor in ObjectRef format
+ */
+ ObjectRef TensorFromTVMArgValueToObjectRef(const TVMArgValue& tensor) const;
+
+ /*!
+ * \brief Get index of outputs in register_file from func code
+ * \return result register index
+ */
+ Index GetResultRegisterIndex() const;
+
+ /*!
+ * \brief Calculate the index of operation which destination is result
+ * \param res_index is the index of op returning result
+ */
+ void CalculatePreResultOpIndex(Index res_index);
+
+ /*!
+ * \brief Collect indices from register_file for output tensors.
+ * It helps to replace output tensors allocated in RunLoop by
+ * tensors pre-allocated outside. Scenario is when `set_output` is used
+ * \param func_name The function's name.
+ */
+ void CollectOutputTensorRegIndices(const std::string& func_name);
Review Comment:
I'd prefer this to return the tensor indices instead of setting some internal state. There's now a lot of stateful functions and it is unclear how they all interact.
##########
src/runtime/vm/vm.cc:
##########
@@ -810,21 +906,90 @@ void VirtualMachine::RunLoop() {
WriteRegister(instr.dst, dst_data);
OpStopHook();
pc_++;
- goto main_loop;
+ break;
}
case Opcode::KillRegister: {
OpStartHook(instr);
WriteRegister(instr.dst, ObjectRef());
OpStopHook();
pc_++;
- goto main_loop;
+ break;
}
default:
LOG(FATAL) << "Unknown instruction opcode: " << int(instr.op);
}
}
}
+void VirtualMachine::WriteAllocatedTensor(const Instruction& instr) {
+ auto shape = std::vector<int64_t>(instr.alloc_tensor.ndim);
+
+ for (uint32_t i = 0; i < instr.alloc_tensor.ndim; ++i) {
+ shape[i] = instr.alloc_tensor.shape[i];
+ }
+
+ auto storage_obj = ReadRegister(instr.alloc_tensor.storage);
+ auto offset = LoadScalarInt(instr.alloc_tensor.offset);
+ auto storage = Downcast<Storage>(storage_obj);
+ auto obj = storage->AllocNDArray(offset, shape, instr.alloc_tensor.dtype);
+ VLOG(2) << "allocated "
+ << RuntimeObject2String(obj, GetDevice(exec_->host_device_index),
+ /*show_contents=*/false);
+
+ WriteRegister(instr.dst, obj);
+}
+
+void VirtualMachine::WriteAllocatedTensorFromOutside(const Instruction& instr) {
+ // External tensor(s) has been already written to the register (instr.dst)
+ auto ex_arr = Downcast<NDArray>(ReadRegister(instr.dst));
+ auto ex_shape = ex_arr.Shape();
+ auto ex_size = ex_shape.size();
+ auto ex_dtype = ex_arr->dtype;
+
+ auto in_size = instr.alloc_tensor.ndim;
+ auto in_dtype = instr.alloc_tensor.dtype;
+ ICHECK_EQ(TypeEqual(in_dtype, ex_dtype), true)
+ << "Data types mismatching for internal and external output tensors";
+
+ bool size_check = false;
+ if (ex_size != in_size) {
+ size_check = true;
+ } else {
+ for (size_t i = 0; i < in_size; ++i) {
+ if (ex_shape[i] != instr.alloc_tensor.shape[i]) {
+ size_check = true;
+ break;
+ }
+ }
+ }
+
+ if (size_check) {
+ // Match element number
+ size_t in_el_num = 1, ex_el_num = 1;
+ for (size_t i = 0; i < ex_size; ++i) {
+ ex_el_num *= ex_shape[i];
+ }
+ for (size_t i = 0; i < in_size; ++i) {
+ in_el_num *= instr.alloc_tensor.shape[i];
+ }
+ ICHECK_EQ(in_el_num, ex_el_num)
+ << "Element number mismatching of internal and external output tensors";
+ if (code_[preresult_op_index_].op == Opcode::ReshapeTensor) {
+ int64_t* dims = instr.alloc_tensor.shape;
+ std::vector<int64_t> ref_shape(dims, dims + int64_t(in_size));
+ auto reshaped_tensor = ex_arr.CreateView(ref_shape, ex_dtype);
+ WriteRegister(instr.dst, reshaped_tensor);
+ } else {
+ LOG_ERROR << "Internal and external output tensor shapes are mismatched";
Review Comment:
```suggestion
LOG(FATAL) << "Internal and external output tensor shapes are mismatched";
```
##########
src/runtime/vm/vm.cc:
##########
@@ -518,13 +584,54 @@ int64_t VirtualMachine::LoadScalarInt(Index r) const {
return result;
}
-void VirtualMachine::RunLoop() {
+Index VirtualMachine::GetResultRegisterIndex() const {
+ Index op_index = 0;
+ while (code_[op_index].op != Opcode::Ret) {
+ ++op_index;
+ }
+
+ return code_[op_index].result;
+}
+
+void VirtualMachine::CalculatePreResultOpIndex(Index res_index) {
+ if (preresult_op_index_ == -1) {
+ preresult_op_index_ = 0;
+ while (code_[preresult_op_index_].dst != res_index) {
+ ++preresult_op_index_;
+ }
+ }
+}
+
+void VirtualMachine::CollectOutputTensorRegIndices(const std::string& func_name) {
+ if (!output_tensor_reg_indices_[func_name].empty()) {
+ return;
+ }
+
+ auto& reg_indices = output_tensor_reg_indices_[func_name];
+ Index res_index = GetResultRegisterIndex();
+ CalculatePreResultOpIndex(res_index);
+ auto& preres_instr = code_[preresult_op_index_];
+ auto op_code = preres_instr.op;
+ if (op_code == Opcode::AllocTensor) {
+ reg_indices.emplace_back(res_index);
+ } else if (op_code == Opcode::AllocADT) {
+ for (Index i = 0; i < preres_instr.num_fields; ++i) {
+ reg_indices.push_back(preres_instr.datatype_fields[i]);
+ }
+ } else if (op_code == Opcode::ReshapeTensor) {
+ reg_indices.push_back(preres_instr.reshape_tensor.tensor);
+ } else {
+ LOG(WARNING) << "Operation " << size_t(op_code) << " is not supported for set_outputs method";
Review Comment:
This should be a fatal.
##########
src/runtime/vm/vm.cc:
##########
@@ -518,13 +584,54 @@ int64_t VirtualMachine::LoadScalarInt(Index r) const {
return result;
}
-void VirtualMachine::RunLoop() {
+Index VirtualMachine::GetResultRegisterIndex() const {
+ Index op_index = 0;
+ while (code_[op_index].op != Opcode::Ret) {
+ ++op_index;
+ }
+
+ return code_[op_index].result;
+}
+
+void VirtualMachine::CalculatePreResultOpIndex(Index res_index) {
+ if (preresult_op_index_ == -1) {
+ preresult_op_index_ = 0;
+ while (code_[preresult_op_index_].dst != res_index) {
+ ++preresult_op_index_;
+ }
+ }
+}
+
+void VirtualMachine::CollectOutputTensorRegIndices(const std::string& func_name) {
+ if (!output_tensor_reg_indices_[func_name].empty()) {
+ return;
+ }
+
+ auto& reg_indices = output_tensor_reg_indices_[func_name];
+ Index res_index = GetResultRegisterIndex();
+ CalculatePreResultOpIndex(res_index);
+ auto& preres_instr = code_[preresult_op_index_];
+ auto op_code = preres_instr.op;
+ if (op_code == Opcode::AllocTensor) {
+ reg_indices.emplace_back(res_index);
+ } else if (op_code == Opcode::AllocADT) {
+ for (Index i = 0; i < preres_instr.num_fields; ++i) {
+ reg_indices.push_back(preres_instr.datatype_fields[i]);
+ }
+ } else if (op_code == Opcode::ReshapeTensor) {
+ reg_indices.push_back(preres_instr.reshape_tensor.tensor);
+ } else {
+ LOG(WARNING) << "Operation " << size_t(op_code) << " is not supported for set_outputs method";
+ }
+}
+
+void VirtualMachine::RunLoop(const std::vector<Index>& output_tensor_reg_indices) {
ICHECK(this->exec_);
ICHECK(this->code_);
pc_ = 0;
Index frame_start = frames_.size();
- while (true) {
- main_loop:
+ bool iterate = true;
+ while (iterate) {
Review Comment:
Remove the change from goto to loop. It doesn't seem necessary for this PR.
##########
python/tvm/runtime/vm.py:
##########
@@ -550,6 +551,24 @@ def invoke_stateful(self, func_name, *args, **kwargs):
self.set_input(func_name, *args, **kwargs)
self._invoke_stateful(func_name)
+ def invoke_with_outputs(self, func_name, *args):
Review Comment:
Can you make this function take input arguments too instead of requiring `set_input`.
##########
src/runtime/vm/vm.cc:
##########
@@ -143,8 +143,15 @@ PackedFunc VirtualMachine::GetFunction(const std::string& name,
} else {
auto it = inputs_.find(func_name);
ICHECK(it != inputs_.end()) << "Input has not been set for function " << func_name;
- const std::vector<ObjectRef>& func_args = it->second;
- *rv = Invoke(func, func_args);
+ const std::vector<ObjectRef>& input_args = it->second;
+ if (set_outputs_enabled_.count(func_name) && set_outputs_enabled_[func_name]) {
+ ICHECK(outputs_.count(func_name))
+ << "Outputs have not been set for function " << func_name;
+ *rv = Invoke(func, input_args, outputs_[func_name]);
+ set_outputs_enabled_[func_name] = false;
Review Comment:
I think you need to clear `outputs_` here to you don't hold an unnecessary reference.
##########
include/tvm/runtime/vm/vm.h:
##########
@@ -281,6 +291,28 @@ class TVM_DLL VirtualMachine : public runtime::ModuleNode {
*/
void SetOneInput(std::string name, const TVMArgValue& tag, const TVMArgValue& tensor);
+ /*!
+ * \brief Set pre-allocated outputs to a function.
+ * \param name The function name
+ * \param args outputs to the function.
+ */
+ void SetOutputs(std::string name, TVMArgs args);
Review Comment:
Clarify that this only applies to the next single `Invoke` call.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r966808686
##########
src/runtime/vm/vm.cc:
##########
@@ -810,21 +906,90 @@ void VirtualMachine::RunLoop() {
WriteRegister(instr.dst, dst_data);
OpStopHook();
pc_++;
- goto main_loop;
+ break;
}
case Opcode::KillRegister: {
OpStartHook(instr);
WriteRegister(instr.dst, ObjectRef());
OpStopHook();
pc_++;
- goto main_loop;
+ break;
}
default:
LOG(FATAL) << "Unknown instruction opcode: " << int(instr.op);
}
}
}
+void VirtualMachine::WriteAllocatedTensor(const Instruction& instr) {
+ auto shape = std::vector<int64_t>(instr.alloc_tensor.ndim);
+
+ for (uint32_t i = 0; i < instr.alloc_tensor.ndim; ++i) {
+ shape[i] = instr.alloc_tensor.shape[i];
+ }
+
+ auto storage_obj = ReadRegister(instr.alloc_tensor.storage);
+ auto offset = LoadScalarInt(instr.alloc_tensor.offset);
+ auto storage = Downcast<Storage>(storage_obj);
+ auto obj = storage->AllocNDArray(offset, shape, instr.alloc_tensor.dtype);
+ VLOG(2) << "allocated "
+ << RuntimeObject2String(obj, GetDevice(exec_->host_device_index),
+ /*show_contents=*/false);
+
+ WriteRegister(instr.dst, obj);
+}
+
+void VirtualMachine::WriteAllocatedTensorFromOutside(const Instruction& instr) {
+ // External tensor(s) has been already written to the register (instr.dst)
+ auto ex_arr = Downcast<NDArray>(ReadRegister(instr.dst));
+ auto ex_shape = ex_arr.Shape();
+ auto ex_size = ex_shape.size();
+ auto ex_dtype = ex_arr->dtype;
+
+ auto in_size = instr.alloc_tensor.ndim;
+ auto in_dtype = instr.alloc_tensor.dtype;
+ ICHECK_EQ(TypeEqual(in_dtype, ex_dtype), true)
+ << "Data types mismatching for internal and external output tensors";
+
+ bool size_check = false;
+ if (ex_size != in_size) {
+ size_check = true;
+ } else {
+ for (size_t i = 0; i < in_size; ++i) {
+ if (ex_shape[i] != instr.alloc_tensor.shape[i]) {
+ size_check = true;
+ break;
+ }
+ }
+ }
+
+ if (size_check) {
+ // Match element number
+ size_t in_el_num = 1, ex_el_num = 1;
+ for (size_t i = 0; i < ex_size; ++i) {
+ ex_el_num *= ex_shape[i];
+ }
+ for (size_t i = 0; i < in_size; ++i) {
+ in_el_num *= instr.alloc_tensor.shape[i];
+ }
+ ICHECK_EQ(in_el_num, ex_el_num)
+ << "Element number mismatching of internal and external output tensors";
+ if (code_[preresult_op_index_].op == Opcode::ReshapeTensor) {
+ int64_t* dims = instr.alloc_tensor.shape;
+ std::vector<int64_t> ref_shape(dims, dims + int64_t(in_size));
+ auto reshaped_tensor = ex_arr.CreateView(ref_shape, ex_dtype);
+ WriteRegister(instr.dst, reshaped_tensor);
+ } else {
+ LOG_ERROR << "Internal and external output tensor shapes are mismatched";
Review Comment:
Thanks! fixed
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: WIP: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r876618145
##########
src/runtime/vm/vm.cc:
##########
@@ -521,13 +603,25 @@ int64_t VirtualMachine::LoadScalarInt(Index r) const {
return result;
}
-void VirtualMachine::RunLoop() {
+Index VirtualMachine::GetResultRegisterIndex() const {
+ Index op_ind = 0;
+ Instruction instr;
+ // TODO(vvchernov): can it be endless loop?
+ do {
+ instr = code_[op_ind++];
+ } while (instr.op == Opcode::Ret);
+
+ return instr.result;
+}
+
+void VirtualMachine::RunLoop(bool set_output_enabled) {
ICHECK(this->exec_);
ICHECK(this->code_);
pc_ = 0;
Index frame_start = frames_.size();
- while (true) {
- main_loop:
+ Index res_reg_index = GetResultRegisterIndex();
Review Comment:
As we can see from RunLoop design, and my tests with multiple outputs showed the same, only one ObjectRef (e.g. ADT) is return as result. It means we need only one index. Nevertheless tests are in progress to check possible issues. I'm not sure that current implementation is ready to merge.
@tmoreau89 what do you think there are any deadlines for this feature?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige merged pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
tkonolige merged PR #11358:
URL: https://github.com/apache/tvm/pull/11358
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
tkonolige commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r967278430
##########
src/runtime/vm/vm.cc:
##########
@@ -518,13 +584,54 @@ int64_t VirtualMachine::LoadScalarInt(Index r) const {
return result;
}
-void VirtualMachine::RunLoop() {
+Index VirtualMachine::GetResultRegisterIndex() const {
+ Index op_index = 0;
+ while (code_[op_index].op != Opcode::Ret) {
+ ++op_index;
+ }
+
+ return code_[op_index].result;
+}
+
+void VirtualMachine::CalculatePreResultOpIndex(Index res_index) {
+ if (preresult_op_index_ == -1) {
+ preresult_op_index_ = 0;
+ while (code_[preresult_op_index_].dst != res_index) {
+ ++preresult_op_index_;
+ }
+ }
+}
+
+void VirtualMachine::CollectOutputTensorRegIndices(const std::string& func_name) {
+ if (!output_tensor_reg_indices_[func_name].empty()) {
+ return;
+ }
+
+ auto& reg_indices = output_tensor_reg_indices_[func_name];
+ Index res_index = GetResultRegisterIndex();
+ CalculatePreResultOpIndex(res_index);
+ auto& preres_instr = code_[preresult_op_index_];
+ auto op_code = preres_instr.op;
+ if (op_code == Opcode::AllocTensor) {
+ reg_indices.emplace_back(res_index);
+ } else if (op_code == Opcode::AllocADT) {
+ for (Index i = 0; i < preres_instr.num_fields; ++i) {
+ reg_indices.push_back(preres_instr.datatype_fields[i]);
+ }
+ } else if (op_code == Opcode::ReshapeTensor) {
+ reg_indices.push_back(preres_instr.reshape_tensor.tensor);
+ } else {
+ LOG(WARNING) << "Operation " << size_t(op_code) << " is not supported for set_outputs method";
+ }
+}
+
+void VirtualMachine::RunLoop(const std::vector<Index>& output_tensor_reg_indices) {
ICHECK(this->exec_);
ICHECK(this->code_);
pc_ = 0;
Index frame_start = frames_.size();
- while (true) {
- main_loop:
+ bool iterate = true;
+ while (iterate) {
Review Comment:
If you want to change it you can submit a separate PR to do so. I don't have enough knowlege to say if there was a good reason behind it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
tkonolige commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r967288970
##########
src/runtime/vm/vm.cc:
##########
@@ -143,8 +143,15 @@ PackedFunc VirtualMachine::GetFunction(const std::string& name,
} else {
auto it = inputs_.find(func_name);
ICHECK(it != inputs_.end()) << "Input has not been set for function " << func_name;
- const std::vector<ObjectRef>& func_args = it->second;
- *rv = Invoke(func, func_args);
+ const std::vector<ObjectRef>& input_args = it->second;
+ if (set_outputs_enabled_.count(func_name) && set_outputs_enabled_[func_name]) {
+ ICHECK(outputs_.count(func_name))
+ << "Outputs have not been set for function " << func_name;
+ *rv = Invoke(func, input_args, outputs_[func_name]);
+ set_outputs_enabled_[func_name] = false;
Review Comment:
The register file holds a reference to the outputs too. So if we clear `outputs_`, they will be released only when the register file is reset.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r966850057
##########
include/tvm/runtime/vm/vm.h:
##########
@@ -339,6 +371,51 @@ class TVM_DLL VirtualMachine : public runtime::ModuleNode {
void SetInputTensorWithIndex(std::vector<ObjectRef>& tensors, // NOLINT(*)
const TVMArgValue& tensor, int index, Device dev);
+ /*!
+ * \brief Convert tensor from TVMArgValue to ObjectRef.
+ * DLTensor and NDArray types are supported.
+ * \param tensor given arg value containing tensor.
+ * \return tensor in ObjectRef format
+ */
+ ObjectRef TensorFromTVMArgValueToObjectRef(const TVMArgValue& tensor) const;
+
+ /*!
+ * \brief Get index of outputs in register_file from func code
+ * \return result register index
+ */
+ Index GetResultRegisterIndex() const;
+
+ /*!
+ * \brief Calculate the index of operation which destination is result
+ * \param res_index is the index of op returning result
+ */
+ void CalculatePreResultOpIndex(Index res_index);
+
+ /*!
+ * \brief Collect indices from register_file for output tensors.
+ * It helps to replace output tensors allocated in RunLoop by
+ * tensors pre-allocated outside. Scenario is when `set_output` is used
+ * \param func_name The function's name.
+ */
+ void CollectOutputTensorRegIndices(const std::string& func_name);
Review Comment:
I've refactored it, but I think the main reason of the problem stays. It is func name role. I do not know somebody uses anything except "main" I did not. I do not know the idea of func name and main thing is code_ does not depend on func name, but RunLoop depends on code_ only. Thus I can assume that using of two func names in front-end leads to error or unexpected behaviour. Looks like that VM design is still raw.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r966776236
##########
src/runtime/vm/vm.cc:
##########
@@ -518,13 +584,54 @@ int64_t VirtualMachine::LoadScalarInt(Index r) const {
return result;
}
-void VirtualMachine::RunLoop() {
+Index VirtualMachine::GetResultRegisterIndex() const {
+ Index op_index = 0;
+ while (code_[op_index].op != Opcode::Ret) {
+ ++op_index;
+ }
+
+ return code_[op_index].result;
+}
+
+void VirtualMachine::CalculatePreResultOpIndex(Index res_index) {
+ if (preresult_op_index_ == -1) {
+ preresult_op_index_ = 0;
+ while (code_[preresult_op_index_].dst != res_index) {
+ ++preresult_op_index_;
+ }
+ }
+}
+
+void VirtualMachine::CollectOutputTensorRegIndices(const std::string& func_name) {
+ if (!output_tensor_reg_indices_[func_name].empty()) {
+ return;
+ }
+
+ auto& reg_indices = output_tensor_reg_indices_[func_name];
+ Index res_index = GetResultRegisterIndex();
+ CalculatePreResultOpIndex(res_index);
+ auto& preres_instr = code_[preresult_op_index_];
+ auto op_code = preres_instr.op;
+ if (op_code == Opcode::AllocTensor) {
+ reg_indices.emplace_back(res_index);
+ } else if (op_code == Opcode::AllocADT) {
+ for (Index i = 0; i < preres_instr.num_fields; ++i) {
+ reg_indices.push_back(preres_instr.datatype_fields[i]);
+ }
+ } else if (op_code == Opcode::ReshapeTensor) {
+ reg_indices.push_back(preres_instr.reshape_tensor.tensor);
+ } else {
+ LOG(WARNING) << "Operation " << size_t(op_code) << " is not supported for set_outputs method";
+ }
+}
+
+void VirtualMachine::RunLoop(const std::vector<Index>& output_tensor_reg_indices) {
ICHECK(this->exec_);
ICHECK(this->code_);
pc_ = 0;
Index frame_start = frames_.size();
- while (true) {
- main_loop:
+ bool iterate = true;
+ while (iterate) {
Review Comment:
Ok, I've reverted this change. But who will do it? I do not think that goto is good solution here (anythere).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r967144294
##########
include/tvm/runtime/vm/vm.h:
##########
@@ -281,6 +291,28 @@ class TVM_DLL VirtualMachine : public runtime::ModuleNode {
*/
void SetOneInput(std::string name, const TVMArgValue& tag, const TVMArgValue& tensor);
+ /*!
+ * \brief Set pre-allocated outputs to a function.
+ * \param name The function name
+ * \param args outputs to the function.
+ */
+ void SetOutputs(std::string name, TVMArgs args);
Review Comment:
I've extended description
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
tkonolige commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r967277533
##########
python/tvm/runtime/vm.py:
##########
@@ -550,6 +551,24 @@ def invoke_stateful(self, func_name, *args, **kwargs):
self.set_input(func_name, *args, **kwargs)
self._invoke_stateful(func_name)
+ def invoke_with_outputs(self, func_name, *args):
Review Comment:
The other option would be `invoke_with_outputs(self, func_name, input_args, output_args)`. Choose whichever you think is best.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on PR #11358:
URL: https://github.com/apache/tvm/pull/11358#issuecomment-1261139767
Hello @tkonolige! Sorry, I was on vacation and the development was stopped for a moment. Additional questions: Where should documentation be? In description for python and native methods or in some tutorials?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on PR #11358:
URL: https://github.com/apache/tvm/pull/11358#issuecomment-1230336149
Hello @tkonolige! Could you review this PR instead of Mark?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r968766346
##########
python/tvm/runtime/vm.py:
##########
@@ -550,6 +551,24 @@ def invoke_stateful(self, func_name, *args, **kwargs):
self.set_input(func_name, *args, **kwargs)
self._invoke_stateful(func_name)
+ def invoke_with_outputs(self, func_name, *args):
Review Comment:
Thanks for tip, I've updated it
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r967121778
##########
python/tvm/runtime/vm.py:
##########
@@ -550,6 +551,24 @@ def invoke_stateful(self, func_name, *args, **kwargs):
self.set_input(func_name, *args, **kwargs)
self._invoke_stateful(func_name)
+ def invoke_with_outputs(self, func_name, *args):
Review Comment:
I can do it in the following way: invoke_with_outputs(self, func_name, *args, **kwargs), where args are output and kwargs are input tensors correspondingly. It is complicated task to split input and output tensors from args. Is this scenario good?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: WIP: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r876607358
##########
src/runtime/vm/vm.cc:
##########
@@ -521,13 +603,25 @@ int64_t VirtualMachine::LoadScalarInt(Index r) const {
return result;
}
-void VirtualMachine::RunLoop() {
+Index VirtualMachine::GetResultRegisterIndex() const {
+ Index op_ind = 0;
+ Instruction instr;
+ // TODO(vvchernov): can it be endless loop?
+ do {
+ instr = code_[op_ind++];
+ } while (instr.op == Opcode::Ret);
Review Comment:
Thanks very much, it was nasty misprint. About endless loop, we do not know size of code_ array and from one side it potentially does not have return op, from other side it also leads to endless loop in RunLoop method. By default I will think that somebody took care of it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
tkonolige commented on PR #11358:
URL: https://github.com/apache/tvm/pull/11358#issuecomment-1261167807
I'd put the documentation in the description for the methods. I think that's where people will look if using them.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r967164258
##########
src/runtime/vm/vm.cc:
##########
@@ -143,8 +143,15 @@ PackedFunc VirtualMachine::GetFunction(const std::string& name,
} else {
auto it = inputs_.find(func_name);
ICHECK(it != inputs_.end()) << "Input has not been set for function " << func_name;
- const std::vector<ObjectRef>& func_args = it->second;
- *rv = Invoke(func, func_args);
+ const std::vector<ObjectRef>& input_args = it->second;
+ if (set_outputs_enabled_.count(func_name) && set_outputs_enabled_[func_name]) {
+ ICHECK(outputs_.count(func_name))
+ << "Outputs have not been set for function " << func_name;
+ *rv = Invoke(func, input_args, outputs_[func_name]);
+ set_outputs_enabled_[func_name] = false;
Review Comment:
It depends on scenario of using output tensors allocated outside. I see two options: 1. In-place scenario: memory for output tensors is allocated once. Each new inference (invoke) writes result in this memory. 2. We should insert new inputs/outputs for each new infer. Just now the second scenario is implemented and in that case `outputs_` can be cleared. But in the first scenario we should store `outputs_`. What do you think should I support the first scenario instead of second one or both ones or keep the current one?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r966811701
##########
src/runtime/vm/vm.cc:
##########
@@ -518,13 +584,54 @@ int64_t VirtualMachine::LoadScalarInt(Index r) const {
return result;
}
-void VirtualMachine::RunLoop() {
+Index VirtualMachine::GetResultRegisterIndex() const {
+ Index op_index = 0;
+ while (code_[op_index].op != Opcode::Ret) {
+ ++op_index;
+ }
+
+ return code_[op_index].result;
+}
+
+void VirtualMachine::CalculatePreResultOpIndex(Index res_index) {
+ if (preresult_op_index_ == -1) {
+ preresult_op_index_ = 0;
+ while (code_[preresult_op_index_].dst != res_index) {
+ ++preresult_op_index_;
+ }
+ }
+}
+
+void VirtualMachine::CollectOutputTensorRegIndices(const std::string& func_name) {
+ if (!output_tensor_reg_indices_[func_name].empty()) {
+ return;
+ }
+
+ auto& reg_indices = output_tensor_reg_indices_[func_name];
+ Index res_index = GetResultRegisterIndex();
+ CalculatePreResultOpIndex(res_index);
+ auto& preres_instr = code_[preresult_op_index_];
+ auto op_code = preres_instr.op;
+ if (op_code == Opcode::AllocTensor) {
+ reg_indices.emplace_back(res_index);
+ } else if (op_code == Opcode::AllocADT) {
+ for (Index i = 0; i < preres_instr.num_fields; ++i) {
+ reg_indices.push_back(preres_instr.datatype_fields[i]);
+ }
+ } else if (op_code == Opcode::ReshapeTensor) {
+ reg_indices.push_back(preres_instr.reshape_tensor.tensor);
+ } else {
+ LOG(WARNING) << "Operation " << size_t(op_code) << " is not supported for set_outputs method";
Review Comment:
done. I considered scenario when results can not be contained into external tensor and default way is used. But client should know that something wrong.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vvchernov commented on a diff in pull request #11358: WIP: [Virtual Machine] Implementation of 'set_output_zero_copy'
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #11358:
URL: https://github.com/apache/tvm/pull/11358#discussion_r876604094
##########
src/runtime/vm/vm.cc:
##########
@@ -537,7 +631,7 @@ void VirtualMachine::RunLoop() {
from_obj = ReadRegister(instr.from);
WriteRegister(instr.dst, from_obj);
pc_++;
- goto main_loop;
+ break;
Review Comment:
Thanks! I always think that simple loop is better than jump approach, it can be checked further. Just now I'm aware about the feature design.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org