Skip to content
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

Bring PackedFunc into TVM Object System #51

Merged
merged 3 commits into from
Feb 2, 2022
Merged

Bring PackedFunc into TVM Object System #51

merged 3 commits into from
Feb 2, 2022

Conversation

cyx-6
Copy link
Contributor

@cyx-6 cyx-6 commented Jan 9, 2022

Bring PackedFunc into TVM Object System

@junrushao
Copy link
Member

CC: @yzhliu @yzh119 @comaniac

@junrushao junrushao changed the title [RFC][Runtime]PackedFunc as Object Bring PackedFunc into TVM Object System Jan 10, 2022
@junrushao
Copy link
Member

CC @wweic


Only one major object is introduced, `PackedFuncObj`, a TVM object in the runtime system (detailed in the next section) which is an ABI stable data structure for packed functions that could be shared across language and DLL boundary.

To avoid API misuse from developers, the `PackedFuncObj` cannot be created or manipulated directly, and the specialization of its creation `make_object<PackedFuncObj>` will be deleted for safety. Instead, the developer-facing class `PackedFunc` remains responsible for creating and managing the object, and for properly setting its content.
Copy link
Member

@zxybazh zxybazh Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please elaborate a bit about the risk of enabling make_object<PackedFuncObj>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Since the PackedFuncObj is an abstract object only, it is not allowed to be created directly, while its derived classes are allowed to do so.

Fix a typo

Co-authored-by: Xiyou Zhou <xiyou.zhou@gmail.com>
@areusch
Copy link
Contributor

areusch commented Jan 12, 2022

@cyx-6 thanks for raising this formal RFC. i chatted with @junrushao1994 and he reminded me that for PackedFunc registered directly with the C++ runtime, there is a fast path in calling them from C++ that avoid re-encoding the arguments into the documented PackedFunc C ABI. using this fast path in the context of a single project makes sense; however, as you point out here in "Unresolved questions," it also means that we may enshrine a de-facto ABI between TVM and other projects which is considerably more complex.

i'm okay with creating this ABI if it provides a clear benefit over the already-defined one. if it doesn't, i would prefer that we do something different, such as extracting packed_func.h into a separate e.g. dlpack library and allow you to use the C++ PackedFunc wrappers in your code directly (and call into TVM code via the C PackedFunc ABI).

it would be great to motivate creating this second ABI with some details about why you can't use the already-defined PackedFunc ABI. performance is a perfectly-good reason, but the RFC says nothing about that. would you mind adding this detail so it is clear why we are doing this? i don't want to create a precedent of going around the PackedFunc ABI since it is presently so universal.

@cyx-6
Copy link
Contributor Author

cyx-6 commented Jan 18, 2022

Thanks @areusch for your questions! Just to provide more context, it's worth mentioning that the majority of DL compilers built on top of the TVM compiler (not only the runtime) use TVM's C++ APIs directly in the project, i.e. all the C++ APIs under include/tvm/ are considered public APIs as a common practice in C++ libraries.

there is a fast path in calling them from C++ that avoid re-encoding the arguments into the documented PackedFunc C ABI

To be clear, the primary motivation of this RFC isn't about performance, but about usability - TVM-based compilers use PackedFunc either created by TVM or the compilers themselves. The missing piece here is that TVM's PackedFunc uses std::function internally right now, which is practically not shareable across C++ DLL boundary. Therefore, this RFC mainly focuses on stabilizing the PackedFunc into a layout-stable TVM object.

i'm okay with creating this ABI if it provides a clear benefit over the already-defined one.

To clarify, This RFC isn't creating new ABIs, just refactoring existing ones. Note that existing TVM-based DL compilers all use C++ APIs instead of C APIs (the generated kernel uses C ABI though). This RFC mainly focuses on standardization of a single specific layout-stable implementation of PackedFunc so that its content could be shared across the DLL boundary - by "refactoring", the impact is limited to the standard TVM runtime and doesn't affect microTVM runtime and the C API is unchanged.

such as extracting packed_func.h into a separate e.g. dlpack library and allow you to use the C++ PackedFunc wrappers in your code directly

I like this proposal. AFAIK there is long-term need from other libraries (e.g. DGL, MXNet) to incorporate with the TVM object/FFI system because it's really easy to use. However, I would say it's separate need from this RFC, but if anyone is interested, please feel free to do so.

it would be great to motivate creating this second ABI with some details about why you can't use the already-defined PackedFunc ABI.

For context, TVM is largely used as a C++ submodule in DL compilers, where all its C++ APIs under include/tvm is treated as public APIs. It's more like a convention for C++ projects.

@junrushao
Copy link
Member

Hey @areusch would you like to follow up on the discussion? Thanks a lot!

Also CC: @comaniac @spectrometerHBH @Hzfengsy if you have further comments

@areusch
Copy link
Contributor

areusch commented Jan 25, 2022

thanks @cyx-6 sorry for the delay

To be clear, the primary motivation of this RFC isn't about performance, but about usability - TVM-based compilers use PackedFunc either created by TVM or the compilers themselves. The missing piece here is that TVM's PackedFunc uses std::function internally right now, which is practically not shareable across C++ DLL boundary. Therefore, this RFC mainly focuses on stabilizing the PackedFunc into a layout-stable TVM object.

I think this is the key part here. can you add this description (about std::function not being able to be passed across the DLL boundary) to the Motivation section? I am good with this RFC after that!

@junrushao
Copy link
Member

@cyx-6 would you mind updating the RFC according to our discussion here and on the forum? Also, please include your PR apache/tvm#10032 in the RFC?

@cyx-6
Copy link
Contributor Author

cyx-6 commented Feb 2, 2022

Thanks for @areusch and @junrushao1994 's suggestions. I have updated the RFC according to discussion here and on the forum.

Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :-)

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @cyx-6 !

@areusch areusch added the status: accepted RFC is accepted label Feb 2, 2022
@areusch areusch merged commit 2e0de6c into apache:main Feb 2, 2022
@junrushao
Copy link
Member

Thank you @cyx-6 @AndrewZhaoLuo @zxybazh all for the discussion! It's very valuable and informative :-)

@areusch
Copy link
Contributor

areusch commented Feb 2, 2022

@cyx-6 the RFC is merged! please open a new GitHub issue in apache/tvm (the "Feature Tracking" type), enumerate the PRs expected in that issue, and cite this RFC so others can find it for context. Thanks again for working on this!

ccjoechou pushed a commit to ccjoechou/tvm-rfcs that referenced this pull request Feb 8, 2022
* [RFC][Runtime] Bring `PackedFunc` into TVM Object System

* Apply suggestions from code review

Fix a typo

Co-authored-by: Xiyou Zhou <xiyou.zhou@gmail.com>

* Apply suggestions from RFC review

Co-authored-by: Xiyou Zhou <xiyou.zhou@gmail.com>
Hzfengsy added a commit to Hzfengsy/tvm that referenced this pull request Sep 2, 2022
Please join me in welcoming Yaxing Cai (@cyx-6) as a new reviewer in TVM. Yaxing has brought the PackedFunc into TVM object system ([RFC-051](apache/tvm-rfcs#51)), designed and implemented the new parser infrastructure for TVMScript and meta-programming ([RFC-079](apache/tvm-rfcs#79))

- [Commits History](https://github.com/apache/tvm/commits?author=cyx-6)
- [Code Review](https://github.com/apache/tvm/pulls?q=reviewed-by%3Acyx-6+)
junrushao pushed a commit to apache/tvm that referenced this pull request Sep 2, 2022
Please join me in welcoming Yaxing Cai (@cyx-6) as a new reviewer in TVM. Yaxing has brought the PackedFunc into TVM object system ([RFC-051](apache/tvm-rfcs#51)), designed and implemented the new parser infrastructure for TVMScript and meta-programming ([RFC-079](apache/tvm-rfcs#79))

- [Commits History](https://github.com/apache/tvm/commits?author=cyx-6)
- [Code Review](https://github.com/apache/tvm/pulls?q=reviewed-by%3Acyx-6+)
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
Please join me in welcoming Yaxing Cai (@cyx-6) as a new reviewer in TVM. Yaxing has brought the PackedFunc into TVM object system ([RFC-051](apache/tvm-rfcs#51)), designed and implemented the new parser infrastructure for TVMScript and meta-programming ([RFC-079](apache/tvm-rfcs#79))

- [Commits History](https://github.com/apache/tvm/commits?author=cyx-6)
- [Code Review](https://github.com/apache/tvm/pulls?q=reviewed-by%3Acyx-6+)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: accepted RFC is accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants