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/01/19 16:31:15 UTC
[GitHub] [tvm] vvchernov opened a new pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
vvchernov opened a new pull request #9980:
URL: https://github.com/apache/tvm/pull/9980
The issue was observed during generation of VirtulaMachine Module on python side and using it on c++ one.
Work case: when we work in one python session the lib (exec) object is generated and used for VirtualMachine compilation. Inside VM the raw pointer to the lib (exec) is copied. Due to the lib object is alive along the session the VM object can use the pointer and execute different methods like "set_input", "invoke" and so on.
Unworkable case: if we use python session for generation VM and try to use it on c++ side it does not work. The reason is the lib object died after the session is closed. Thus VM uses the pointer to the spoiled object and it leads to failure.
To fix this problem Executable* pointer was replaced by solid ObjectPtr<Executable> and other depended methods were corrected.
The weak place is Downcasting From ObjectPtr<Object> to ObjectPtr<Executable> due to it is prohibited by special check in direct constructor (it is commented in the PR).
@jwfromm @tmoreau89 please see.
--
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 change in pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov commented on a change in pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#discussion_r788033479
##########
File path: include/tvm/runtime/vm/vm.h
##########
@@ -300,7 +300,7 @@ class VirtualMachine : public runtime::ModuleNode {
/*! \brief The special return register. */
ObjectRef return_register_;
/*! \brief The executable the VM will operate on. */
- Executable* exec_;
+ ObjectPtr<Executable> exec_;
Review comment:
I have got idea, I will try
--
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] jroesch commented on a change in pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#discussion_r788017534
##########
File path: include/tvm/runtime/object.h
##########
@@ -372,8 +372,8 @@ class ObjectPtr {
template <typename U>
ObjectPtr(const ObjectPtr<U>& other) // NOLINT(*)
: ObjectPtr(other.data_) {
- static_assert(std::is_base_of<T, U>::value,
- "can only assign of child class ObjectPtr to parent");
Review comment:
Agree with TQ this should not be remove
--
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] tqchen commented on a change in pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#discussion_r790085393
##########
File path: src/runtime/vm/executable.cc
##########
@@ -86,7 +86,7 @@ PackedFunc Executable::GetFunction(const std::string& name, const ObjectPtr<Obje
} else if (name == "vm_load_executable") {
return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) {
auto vm = make_object<VirtualMachine>();
- vm->LoadExecutable(this);
+ vm->LoadExecutable(ObjectRef(sptr_to_self));
Review comment:
GetObjectPtr<Executable>(this);
##########
File path: include/tvm/runtime/vm/vm.h
##########
@@ -174,7 +174,7 @@ class VirtualMachine : public runtime::ModuleNode {
* \brief load the executable for the virtual machine.
* \param exec The executable.
*/
- virtual void LoadExecutable(Executable* exec);
+ virtual void LoadExecutable(const ObjectRef& exec);
Review comment:
consider change to `ObjectPtr<Executable>` which can be constructed via https://github.com/apache/tvm/blob/main/include/tvm/runtime/object.h#L877
--
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] tqchen commented on pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1019007992
cc @YuchenJin @yongwww since this is VM related
--
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] tqchen merged pull request #9980: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #9980:
URL: https://github.com/apache/tvm/pull/9980
--
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] tmoreau89 commented on pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1022762448
@vvchernov there seems to be a CI issue - from the looks of it it might be flaky - can you re-trigger?
--
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 #9980: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1024104646
Thanks @driazati ! It works
--
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] jwfromm commented on pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
jwfromm commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1023508450
@areusch or @driazati is there a way around the CI error this PR is hitting? Seems like the doc generation is being flaky.
--
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] tmoreau89 commented on pull request #9980: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1024377962
At first sight the CI error seems completely unrelated to the PR...
--
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 #9980: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1025793925
Thanks @tqchen for your help, I've used your approch with GetObjectPtr. It looks like CI tests should pass soon. May be you recheck my last changes.
--
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] tqchen commented on a change in pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#discussion_r787989010
##########
File path: include/tvm/runtime/object.h
##########
@@ -372,8 +372,8 @@ class ObjectPtr {
template <typename U>
ObjectPtr(const ObjectPtr<U>& other) // NOLINT(*)
: ObjectPtr(other.data_) {
- static_assert(std::is_base_of<T, U>::value,
- "can only assign of child class ObjectPtr to parent");
Review comment:
Likely we want to keep this static assert
--
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 change in pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov commented on a change in pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#discussion_r790524396
##########
File path: src/runtime/vm/executable.cc
##########
@@ -86,7 +86,7 @@ PackedFunc Executable::GetFunction(const std::string& name, const ObjectPtr<Obje
} else if (name == "vm_load_executable") {
return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) {
auto vm = make_object<VirtualMachine>();
- vm->LoadExecutable(this);
+ vm->LoadExecutable(ObjectRef(sptr_to_self));
Review comment:
As I know sptr_to_self is already `ObjectPtr<Object>`, if it is immediatly `ObjectPtr<Executable>` or can be simply converted to `ObjectPtr<Executable>` it will be nice, but it is not so. Unfortunately your approach is bad due to GetObjectPtr(...) creates new ObjectPtr<> object which contains copy of raw pointer. There is no any connection between origin and new ObjectPtr. It means that if origin object was released (e.g. when python session where it was generated is closed) we will have pointer to broken object and can not use it. just my PR solves this problem, but your approach does not.
##########
File path: src/runtime/vm/executable.cc
##########
@@ -86,7 +86,7 @@ PackedFunc Executable::GetFunction(const std::string& name, const ObjectPtr<Obje
} else if (name == "vm_load_executable") {
return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) {
auto vm = make_object<VirtualMachine>();
- vm->LoadExecutable(this);
+ vm->LoadExecutable(ObjectRef(sptr_to_self));
Review comment:
As I know sptr_to_self is already `ObjectPtr<Object>`, if it is immediatly `ObjectPtr<Executable>` or can be simply converted to `ObjectPtr<Executable>` it will be nice, but it is not so. Unfortunately your approach is bad due to GetObjectPtr(...) creates new ObjectPtr<> object which contains copy of raw pointer. There is no any connection between origin and new ObjectPtr. It means that if origin object was released (e.g. when python session where it was generated is closed) we will have pointer to broken object and can not use it. just my PR solves this problem, but your approach does not as I think.
--
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 removed a comment on pull request #9980: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov removed a comment on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1025804573
Thanks @tqchen for your help, I've used your approch with GetObjectPtr. It looks like CI tests should pass soon. May be you recheck my last changes.
--
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 #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1018711574
Hello @tqchen @jroesch Are there any additional comments, questions?
--
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] tqchen commented on pull request #9980: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1024509306
@vvchernov would be great if you can confirm my latest comment, which should be a better fix
```
Sorry for the delayed reply. sptr_to_self actually is guanranteed to point to this in this particular case. So actually GetObjectPtr<Executable>(...) should work in this case. This is mainly because ObjectPtr is an intrusive pointer that directly leverages the object data structure.
To confirm, we can add ICHECK(sptr_to_self.get() == this);
``
--
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 #9980: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1025804573
Thanks @tqchen for your help, I've used your approch with GetObjectPtr. It looks like CI tests should pass soon. May be you recheck my last changes.
--
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 change in pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov commented on a change in pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#discussion_r790513538
##########
File path: include/tvm/runtime/vm/vm.h
##########
@@ -174,7 +174,7 @@ class VirtualMachine : public runtime::ModuleNode {
* \brief load the executable for the virtual machine.
* \param exec The executable.
*/
- virtual void LoadExecutable(Executable* exec);
+ virtual void LoadExecutable(const ObjectRef& exec);
Review comment:
It was advice from @jroesch to use ObjectRef. And it seems the simplest way from possible ones. I agree that using of ObjectPtr<Executable> is preferable but to get it correctly is not so simple task (see below).
--
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 change in pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov commented on a change in pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#discussion_r790524396
##########
File path: src/runtime/vm/executable.cc
##########
@@ -86,7 +86,7 @@ PackedFunc Executable::GetFunction(const std::string& name, const ObjectPtr<Obje
} else if (name == "vm_load_executable") {
return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) {
auto vm = make_object<VirtualMachine>();
- vm->LoadExecutable(this);
+ vm->LoadExecutable(ObjectRef(sptr_to_self));
Review comment:
As I know sptr_to_self is already `ObjectPtr<Object>`, if it is immediatly `ObjectPtr<Executable>` or can be simply converted to `ObjectPtr<Executable>` it will be nice, but it is not so. Unfortunately your approach is bad due to GetObjectPtr(...) creates new ObjectPtr<> object which contains copy of raw pointer. There is no any connection between origin and new ObjectPtr. It means that if origin object was released (e.g. when python session where it was generated is closed) we will have pointer to broken object and can not use it. just my PR solves this problem, but your approach does not.
--
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 #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1022163942
Hello @tqchen @jroesch @yongwww @YuchenJin, any questions? May be we merge this PR?
--
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 change in pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov commented on a change in pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#discussion_r788033479
##########
File path: include/tvm/runtime/vm/vm.h
##########
@@ -300,7 +300,7 @@ class VirtualMachine : public runtime::ModuleNode {
/*! \brief The special return register. */
ObjectRef return_register_;
/*! \brief The executable the VM will operate on. */
- Executable* exec_;
+ ObjectPtr<Executable> exec_;
Review comment:
I have gotten idea, I will try
--
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] tqchen commented on pull request #9980: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1026298253
Thanks @vvchernov !
--
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] tmoreau89 commented on pull request #9980: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1024377474
@driazati does this error seem like a flaky issue to you?
> worker 'gw3' crashed while running 'tests/python/contrib/test_ethosu/test_codegen.py::test_mean[ifm_shape8-axis8-False-False-ethos-u55-256]'
--
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] tqchen commented on a change in pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#discussion_r793190074
##########
File path: src/runtime/vm/executable.cc
##########
@@ -86,7 +86,7 @@ PackedFunc Executable::GetFunction(const std::string& name, const ObjectPtr<Obje
} else if (name == "vm_load_executable") {
return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) {
auto vm = make_object<VirtualMachine>();
- vm->LoadExecutable(this);
+ vm->LoadExecutable(ObjectRef(sptr_to_self));
Review comment:
Sorry for the delayed reply. sptr_to_self actually is guanranteed to point to `this` in this particular case. So actually `GetObjectPtr<Executable>(...)` should work in this case. This is mainly because ObjectPtr is an intrusive pointer that directly leverages the object data structure.
To confirm, we can add `ICHECK(sptr_to_self.get() == this);`
--
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 change in pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov commented on a change in pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#discussion_r790524396
##########
File path: src/runtime/vm/executable.cc
##########
@@ -86,7 +86,7 @@ PackedFunc Executable::GetFunction(const std::string& name, const ObjectPtr<Obje
} else if (name == "vm_load_executable") {
return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) {
auto vm = make_object<VirtualMachine>();
- vm->LoadExecutable(this);
+ vm->LoadExecutable(ObjectRef(sptr_to_self));
Review comment:
As I know sptr_to_self is already ObjectPtr<Object>, if it is immediatly ObjectPtr<Executable> or can be simply converted to ObjectPtr<Executable> it will be nice, but it is not so. Unfortunately your approach is bad due to GetObjectPtr(...) creates new ObjectPtr<> object which contains copy of raw pointer. There is no any connection between origin and new ObjectPtr. It means that if origin object was released (e.g. when python session where it was generated is closed) we will have pointer to broken object and can not use it. just my PR solves this problem, but your approach does not.
--
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] jroesch commented on a change in pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#discussion_r788019743
##########
File path: include/tvm/runtime/vm/vm.h
##########
@@ -300,7 +300,7 @@ class VirtualMachine : public runtime::ModuleNode {
/*! \brief The special return register. */
ObjectRef return_register_;
/*! \brief The executable the VM will operate on. */
- Executable* exec_;
+ ObjectPtr<Executable> exec_;
Review comment:
If you get Executable an ObjectRef wrapper like the rest of the code base you can then use the down casting machinery built on-top of ObjectRef. It might make working with this type easier.
--
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 change in pull request #9980: WIP: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
vvchernov commented on a change in pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#discussion_r790524396
##########
File path: src/runtime/vm/executable.cc
##########
@@ -86,7 +86,7 @@ PackedFunc Executable::GetFunction(const std::string& name, const ObjectPtr<Obje
} else if (name == "vm_load_executable") {
return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) {
auto vm = make_object<VirtualMachine>();
- vm->LoadExecutable(this);
+ vm->LoadExecutable(ObjectRef(sptr_to_self));
Review comment:
As I know sptr_to_self is already `ObjectPtr<Object>`, if it is immediatly `ObjectPtr<Executable>` or can be simply converted to `ObjectPtr<Executable>` it will be nice, but it is not so. Unfortunately your approach is bad due to GetObjectPtr(...) creates new ObjectPtr<> object which contains copy of raw pointer. There is no any connection between origin and new ObjectPtr. It means that if origin object was released (e.g. when python session where it was generated is closed) we will have pointer to broken object and can not use it. just my PR solves this problem, but your approach does not as I think.
--
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] driazati commented on pull request #9980: [VirtualMachine] fix raw pointer using by VirtualMachine
Posted by GitBox <gi...@apache.org>.
driazati commented on pull request #9980:
URL: https://github.com/apache/tvm/pull/9980#issuecomment-1023586900
Docs were broken for a while earlier, @vvchernov could you rebase (`git fetch origin && git rebase origin/main`) one more time? The latest commit (https://github.com/apache/tvm/commit/fa317edf78ec9555a606fe57052d587c90a27375) should have gotten everything back in working order
--
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