-
Notifications
You must be signed in to change notification settings - Fork 49
doc: abi overview #402
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
doc: abi overview #402
Conversation
Summary of ChangesHello @junrushao, 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 enhances the project's documentation by introducing a detailed and structured overview of the TVM-FFI Application Binary Interface. The new documentation clarifies the fundamental design principles, data structures, and interaction patterns of the ABI, supported by concrete C code examples. This addition aims to provide developers with a clearer understanding of how to work with TVM-FFI at a low level, improving the accessibility and maintainability of the system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
Summary of ChangesHello @junrushao, 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 enhances the TVM-FFI documentation by introducing a new, comprehensive overview of its Application Binary Interface (ABI). The new document, written in reStructuredText and supported by concrete C code examples, aims to clarify the fundamental principles, data structures, and interaction patterns of the FFI system. This change improves the accessibility and understanding of TVM-FFI's low-level mechanisms for developers, ensuring a more robust and maintainable codebase. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 comprehensive documentation for the TVM-FFI ABI, complete with low-level C code examples. The changes are well-organized, adding a new overview page, example code, and updating existing documentation and header files to support the new content. My review has identified a critical bug in an example C function and a few minor areas for improvement in the documentation for clarity and completeness.
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 a comprehensive ABI overview document, which is a great addition. I've found a few areas in the new documentation that could be improved for clarity and correctness, including a grammatical error, a 'TBA' placeholder, and a minor wording issue. More importantly, I've identified a bug in one of the C code examples where a function incorrectly handles type casting, which could lead to incorrect behavior. My review includes suggestions to fix these issues.
eb6b498 to
ee05592
Compare
3c8aa93 to
23351c1
Compare
|
/gemini review |
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 is a significant and valuable documentation refactoring. It introduces a new, comprehensive document, abi_overview.rst, to serve as a central reference for the TVM FFI C ABI. Consequently, other related documents have been streamlined by removing duplicated C-level details and adding links to this new central page. This change greatly improves the structure and maintainability of the documentation. The new document is well-written and effectively uses literalinclude to embed code snippets from a dedicated example file, which is an excellent practice. My review includes a couple of suggestions to further enhance the clarity of the new abi_overview.rst document. Overall, this is a high-quality contribution.
74acc9b to
a50c8db
Compare
|
/gemini Review |
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 is a significant and valuable documentation refactoring. It centralizes the TVM FFI ABI low-level details into a new abi_overview.rst document, which improves maintainability and clarity. The old markdown version is removed, and other documentation files are updated to link to the new central document, removing duplicated content. A new C file with code examples is also added to support the new documentation.
The changes are well-executed. I have a couple of suggestions to improve the new code examples in abi_overview.rst by adding explicit notes about resource management. This will help prevent potential resource leaks for users who copy-paste the examples.
e449e34 to
0d62310
Compare
0d62310 to
60259e2
Compare
|
some followup notes |
| - ``result`` (``TVMFFIAny*``): Owning :cpp:class:`~tvm::ffi::Any` output value | ||
| - Return value: ``0`` for success; ``-1`` or ``-2`` for errors (see :ref:`sec:exception`) | ||
|
|
||
| See :ref:`sec:function-calling-convention` for more details. |
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.
need note
Before calling the function, caller must set `result->type_index` to be kTVMFFINone, or any type index that do not corresponds to an on-heap object.
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.
It's covered in another doc: https://tvm.apache.org/ffi/concepts/func_module.html#tvm-ffi-c-abi. but happy to emphasize it
60259e2 to
e38261d
Compare
No description provided.