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 the C++ ONNX importer on par with onnx_importer.py #3960

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

giacs-epic
Copy link
Contributor

@giacs-epic giacs-epic commented Jan 15, 2025

This PR heavily refactors the C++ importer to make it a viable (and faster) alternative to the Python one for those who need to import ONNX models and want to avoid depending on the python ecosystem.

  • The C++ importer now outputs the same exact mlir as the Python one (tested on alt_e2eshark test suite). Achieving perfect output matches required to introduce an associative map iterable according to insertion order (to mimic Dict in Python).
  • The code tries to mirror 1-to-1 the Python counterpart whenever possible/convenient.
  • Adds support for embedding ONNX external data in the mlir. This functionality is not part of torch-mlir's onnx_importer.py but of IREE's import_onnx.
  • Efforts have been made to remove the direct dependency on LLVM support lib. There is however a transitive dependency on such lib through MLIRCAPIIR and TorchMLIRCAPI (MLIR libraries uniformly depend on LLVMSupport).

@@ -18,19 +18,29 @@
// this kind of style.

#include "mlir-c/IR.h"
#include "mlir/Support/LLVM.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't expose LLVM support lib internals as part of the public API.

Looks to be just for FailureOr. Best to find a simpler plain C++ way to do that.

I'd prefer to not introduce the LLVM support into the implementation either. The reason is based on experience: This code will likely end up getting used in a variety of places, some of which will be late binding. The LLVM support library is poorly factored and every time we've used it in "highly traveled" utility code like this, I end up fighting an endless list of ODR violations and other bugs. I don't think it pays for itself when used as part of a standalone library like this. Glancing through the implementation, it is using a few conveniences but most could just be inlined and not introduce the dep. The rest (i.e. ArrayRef, string stuff, etc) have easy alternatives in C++17 and 20. I'd consider using C++20 features for code like this to be a less bad way to go than pulling in the support library for some minor ergonomic stuff.

@giacs-epic giacs-epic changed the title Update C onnx importer Bring the C++ ONNX importer on par with onnx_importer.py Feb 17, 2025
@giacs-epic giacs-epic marked this pull request as ready for review February 17, 2025 13:36
@zjgarvey zjgarvey self-requested a review February 20, 2025 01:02
@zjgarvey
Copy link
Collaborator

@vinayakdsci I'm going to take a look at this tomorrow. If you also have a free cycle to review, I'd appreciate it.

@zjgarvey
Copy link
Collaborator

Thanks for all of this work on bringing up the C++ importer.

I took a first pass, and I'll need a little more time to review. I'm not sure how long it will take to get this merged, so I was wondering if you would be willing to factor out the python importer bug fixes to a different PR so we can expedite merging those separately (they seem rather important in their own right).

I'm going to take a closer look, then clone and test some of it out tomorrow.

Do you have a branch with some of the SHARK-TestSuite changes you are using to compare the mlir output against the python importer?

@giacs-epic
Copy link
Contributor Author

Thanks for all of this work on bringing up the C++ importer.

I took a first pass, and I'll need a little more time to review. I'm not sure how long it will take to get this merged, so I was wondering if you would be willing to factor out the python importer bug fixes to a different PR so we can expedite merging those separately (they seem rather important in their own right).

I'm going to take a closer look, then clone and test some of it out tomorrow.

Do you have a branch with some of the SHARK-TestSuite changes you are using to compare the mlir output against the python importer?

Thanks for starting looking at this, I understand it will take a while given its size.
I opened this PR with the onnx_importer.py fixes: #4037
Here's the changes to SHARK-TestSuite I've been using to compare outputs: giacs-epic/SHARK-TestSuite#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants