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

[CMSIS-NN] Remove support for the old CMSIS NN project #13760

Merged
merged 1 commit into from
Jan 17, 2023

Conversation

NicolaLancellotti
Copy link
Contributor

@NicolaLancellotti NicolaLancellotti commented Jan 11, 2023

Pr: #13656 adds support for the new CMSIS NN project

After the docker image is updated we can remove support for the old CMSIS NN project.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Jan 11, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

@NicolaLancellotti
Copy link
Contributor Author

@mehrdadh @ashutosh-arm

@mehrdadh
Copy link
Member

I'm gonna update the docker image to 20.04 once this is merged:
#13736
then you can open this PR

@ashutosh-arm
Copy link
Contributor

ashutosh-arm commented Jan 11, 2023

@mehrdadh we would need to merge #13762 before running tests with latest Cortex-M docker image. Without that change, some Zephyr tests might fail.

@mehrdadh
Copy link
Member

You need to update the image as well, otherwise it doesn't pass the tests. You can use this image that I just pushed
https://hub.docker.com/layers/tlcpack/ci-cortexm/20230111-165944-a9c6f137d/images/sha256-d6b549d9dd5049b717d8e119477c184b18cc52182169c628e050daa146a98776?context=explore

Also, to actually test the PR you need to also commit this to a TVM branch. Let me know if you don't have access and I will do it.

@mehrdadh
Copy link
Member

@ashutosh-arm I don't think we need #13762 to merge this. Because Zephyr project API checks for dependency to CMSIS_NN and if the project is not dependent it doesn't need to add CMSIS_NN to generated project

@mehrdadh
Copy link
Member

Also we could separate updating the image from this PR, I think that should be fine as well

@mehrdadh
Copy link
Member

#13764 is updating the image.

Pr: apache#13656 adds support for the new CMSIS NN project

Change-Id: I28e99fd511b42d9a77ebc6bdc085fc5ffc6b533a
@NicolaLancellotti NicolaLancellotti marked this pull request as ready for review January 17, 2023 14:20
Copy link
Contributor

@ashutosh-arm ashutosh-arm left a comment

Choose a reason for hiding this comment

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

Thanks @NicolaLancellotti. LGTM!

@NicolaLancellotti
Copy link
Contributor Author

I rebased the pr, and CI is now green.

@mehrdadh mehrdadh merged commit c9b4016 into apache:main Jan 17, 2023
fzi-peccia pushed a commit to fzi-peccia/tvm that referenced this pull request Mar 27, 2023
Pr: apache#13656 adds support for the new CMSIS NN project
After the docker image is updated we can remove support for the old CMSIS NN project.
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.

4 participants