-
Notifications
You must be signed in to change notification settings - Fork 35
[Feature] Support vanilla C++ STL in FFI. #228
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @DarkSharpness, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly extends the Foreign Function Interface (FFI) capabilities by adding direct support for several standard C++ STL containers, including Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces support for several C++ STL containers within the FFI layer, a valuable feature that enhances compatibility and simplifies usage. The implementation is well-structured, primarily using TypeTraits specializations for std::array, std::vector, std::tuple, std::optional, and std::variant. The modifications in function_details.h aim to correctly handle argument passing by reference. While the overall approach is solid, I've identified a critical bug in the std::optional TypeTraits implementation that would cause infinite recursion, and a performance issue related to const reference handling in function_details.h. My review includes specific suggestions to address these points.
|
please split the core changes likely stl should go into extra. The main reason is that while stl can be helpful sometimes, the tradeoff is when present in cases like object, we can no longer to near zero cost access in static languages like rust via directly ptr and access (otherwise it have to be a call to reflection getter that does conversion under the hood, so we should always encourage ffi types when possible (and document such rationales in stl.h). |
I agree that stl should go into extra. The main goal of this PR is to introduce a pybind/PyTorch-style interface that provides greater flexibility for user-defined conversions. In performance-critical paths, however, FFI types should always take priority. This change only performs a one-time transformation for convenience and potential backward compatibility with existing code. |
|
@DarkSharpness that sounds good, can we still move function_details.h chaneg into a separate PR? mainly want to make sure core changes are clearly scoped |
|
@tqchen now in #229 . Also tried to fix the More tests of this PR is working in progress. |
Related PR #228. We now support all kinds of argument type (T, const T, T&, const T&, T&&, const T&& have been tested) in C++ exported functions.
0ee3492 to
7749d6f
Compare
In the old code, we reuse traits like `TypeTrait<Array<T>>` and `TypeTrait<Tuple<T>>`. This may result in unwanted copy between FFI containers and STL containers. We fix all of them.
|
cc @tqchen . We now support For STL containers, when passed from Python to C++, we will always construct from the Python object. When passed from C++ to Python, we will construct the corresponding object (e.g. We also allow types that doesn't enable storage (e.g. Finally, do you think I should split the |
|
Thanks @DarkSharpness , would be good to get some reviews, cc @junrushao @Ubospica @Kathryn-cat |
Similar to pybind, we add a
stl.hwhich supportarray,vector,tuple,optionalandvariant. After this file is included, users can use native C++ components, which could hopefully improve compatibility and reduce manual effort in converting between tvm::ffi components to C++ STL components.We also modify the
function_detail.ha little, so that we support all kinds of argument type (T,const T,T&,const T&,T&&,const T&&have been tested) in C++ exported functions.Example code:
Python part: