-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RUNTIME][REFACTOR] Use object protocol to support runtime::Module #4289
Conversation
Will do this Sunday |
89eaafd
to
75ddf51
Compare
@@ -71,7 +70,11 @@ class Module { | |||
* \note Cyclic dependency is not allowed among modules, | |||
* An error will be thrown when cyclic dependency is detected. | |||
*/ | |||
TVM_DLL void Import(Module other); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we intentionally remove the TVM_DLL here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DLL function was moved to the Node class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR migrates tvm::runtime::Module
from std::shared_ptr
to the new TVM object protocol. The advantage of unifying this is obvious - no need to say.
One possible issue is that it introduces an internal header src/runtime/ffi_utils.h
. The naming is somewhat vague...and why not simply put those 3 functions into include/tvm/runtime/object.h
?
The main reason for the ffi_util.h is to keep these functionalities internal, as we do not want to expose these protected field of objects in most cases. |
@tqchen The use case of the class |
I thought a bit about Object::Internal, but eventually chose the name FFIUtil because it also has a name for runtime::Module. |
after thinking a bit more, i agree that ObjectInternal could indeed be a better name, I just pushed a change to do the rename, thanks @junrushao1994 |
Previously runtime::Module was supported using shared_ptr. This PR refactors the codebase to use the Object protocol. It will open doors to allow easier interpolation between Object containers and module in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ping @yzhliu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…pache#4289) Previously runtime::Module was supported using shared_ptr. This PR refactors the codebase to use the Object protocol. It will open doors to allow easier interpolation between Object containers and module in the future.
…pache#4289) Previously runtime::Module was supported using shared_ptr. This PR refactors the codebase to use the Object protocol. It will open doors to allow easier interpolation between Object containers and module in the future.
Previously runtime::Module was supported using shared_ptr.
This PR refactors the codebase to use the Object protocol.
It will open doors to allow easier interpolation between
Object containers and module in the future.
RFC #4286