-
Notifications
You must be signed in to change notification settings - Fork 27
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
Split OnnxRuntime into high-level and low-level generated code #323
Conversation
👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into |
@mcimadamore This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
@mcimadamore this pull request can not be integrated into git checkout onnx_jextract
git fetch https://git.openjdk.org/babylon.git code-reflection
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge code-reflection"
git push |
I've added some changes to make sure that the segments returned by the high-level API keep the automatic arena reachable. There are two places where I've added a
|
Fix lifetime of output address params
fe746c0
to
343501d
Compare
@mcimadamore Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
/integrate |
Going to push as commit 4752b37. |
@mcimadamore Pushed as commit 4752b37. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This PR reorganizes the code in the OnnxRuntime class. It now contains an higher-level implementation of the onnx C API, which is separate from the low-level bindings. These bindings are defined in the new
foreign
package, and have been mechanically generated with a tweaked version ofjextract
which:The generated code is still quite big (OrtApi struct contains 10K LoC). I can make the bindings smaller if we only want to focus on the handful of functions we use today -- but in its current form, the low-level bindings are "complete" and support the full C API.
The main changes in OnnxRuntime are as follows:
reinterpret
methods (which use the correct struct layout size)When doing this exercise, I uncovered an issue with the MHs for a couple of functions (
ReleaseEnv
andReleaseSession
) being incorrect -- the underlying function pointers for these are void (so no result) and they don't accept a trailingret
segment. Also callingcheckStatus
on them makes no sense (again, because these functions don't return a status).Progress
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/babylon.git pull/323/head:pull/323
$ git checkout pull/323
Update a local copy of the PR:
$ git checkout pull/323
$ git pull https://git.openjdk.org/babylon.git pull/323/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 323
View PR using the GUI difftool:
$ git pr show -t 323
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/babylon/pull/323.diff
Using Webrev
Link to Webrev Comment