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

Move to more recent LLVM commit ID #64

Merged
merged 8 commits into from
Apr 1, 2020
Merged

Conversation

byronChanguion
Copy link
Contributor

@byronChanguion byronChanguion commented Mar 31, 2020

This change moves to a more recent LLVM commit ID and therefore allows onnx-mlir to work with latest MLIR changes.

Note that the CI build will need to be updated to the LLVM commit id specified in utils/install-mlir.sh

@CLAassistant
Copy link

CLAassistant commented Mar 31, 2020

CLA assistant check
All committers have signed the CLA.

# Check out a specific branch that is known to work with ONNX MLIR.
cd llvm-project && git checkout 076475713c236081a3247a53e9dbab9043c3eac2 && cd ..
mkdir llvm-project/build
if [ ! -d llvm-project ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason for introducing these conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just made it easier to keep re-running the script so it didn't error-out if the cloned repo existed (e.g. testing out a different commit id). I can remove if required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be great. In a normal use case you shouldn't have to run this script more than once.

Since you tested several commit IDs, does this mean that the current commit ID used in your patch is not the latest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it was just the latest when I looked at it last week.
Let me update to the latest commit id I can and revert back the extra conditions I added. Once I've tested I'll update this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds great! Thanks a lot!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Commit id changed to latest master (as of a couple hours ago), and conditions rolled back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@doru1004 , how do I fix the doc errors reported from the ci?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, got it.

@doru1004
Copy link
Collaborator

You will also have to bump up the LLVM version in .circleci/config.yml otherwise the buildbot will use a cached version of the llvm-project instead of the new one. Currently it's V9 you should bump that up to V10 and then I think your branch will build fine.

Copy link
Collaborator

@doru1004 doru1004 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the patch. LGTM!

@byronChanguion
Copy link
Contributor Author

Thanks a lot for the patch. LGTM!

No problem. Since I don't have write access, can you or someone with access merge this PR into master for me?

@doru1004 doru1004 merged commit b65e773 into onnx:master Apr 1, 2020
mgehre-amd referenced this pull request in Xilinx/onnx-mlir Feb 9, 2023
…o_tosa

feat: add conversion of leaky relu from onnx to tosa
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