[CMSIS-NN] Support CMSIS NN from new GitHub location #13656

NicolaLancellotti · 2022-12-23T13:53:10Z

CMSIS NN has been moved out of the CMSIS project into a new GitHub project.
This pr adds support for CMSIS NN from this new GitHub location.
Both CMSIS and CMSIS NN are now downloaded in /opt/arm/ethosu/cmsis

tvm-bot · 2022-12-23T13:53:13Z

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.

cc @areusch, @ashutosh-arm _{See #10317 for details}

_{Generated by tvm-bot}

ashutosh-arm · 2022-12-23T14:05:41Z

Thanks @NicolaLancellotti for this changes. Looks good to me overall. A small request though - is it possible to checkout the repository with the name CMSIS-NN instead of NN under /opt/arm/ethosu/cmsis?

NicolaLancellotti · 2022-12-23T15:20:23Z

Thanks @NicolaLancellotti for this changes. Looks good to me overall. A small request though - is it possible to checkout the repository with the name CMSIS-NN instead of NN under /opt/arm/ethosu/cmsis?

Done.

mehrdadh

@NicolaLancellotti thanks for this PR!
Could you please add this change to docker script installation?
https://github.com/apache/tvm/blob/main/docker/install/ubuntu_install_cmsis.sh

NicolaLancellotti · 2023-01-09T13:31:53Z

I updated the pr with three commits:

First, I supported CMSIS NN from the new GitHub location, as previously done.
Second, I prevented the definition of the cmsis_path project API option from an environment variable. Before, thanks to the environment variable, the cmsis_path option was always enabled. That was not a problem, but now CMSIS NN uses a new header (arm_acle.h) which is not always present, so we need to explicitly enable CMSIS when we need it.
In the end, I re-added support for the old location of CMSIS NN because the docker image is not yet updated, and we need the tests to pass to accept this pr. In this way, tvm will use the new CMSIS NN project when we will update the docker image, but for now, it uses the old one. I'll create a pr that reverts this last commit when the docker image is updated.

Change-Id: I2c39bc671a8a2583f8e271cae98f61f8cabd1ab9

Change-Id: I1585ec3b31626bde5fa5c70224b1c70b48ceeadf

Change-Id: If6237cc6903d03d0e3ff04e849a6812f1980244a

ashutosh-arm · 2023-01-09T17:02:24Z

docker/install/ubuntu_install_ethosu_driver_stack.sh

@@ -85,6 +85,9 @@ cmake -DCMAKE_TOOLCHAIN_FILE=${ethosu_dir}/core_platform/cmake/toolchain/arm-non
 make

 # Build NN Library
+mkdir ${CMSIS_PATH}/CMSIS-NN/build/ && cd ${CMSIS_PATH}/CMSIS-NN/build/
+cmake .. -DCMAKE_TOOLCHAIN_FILE=${ethosu_dir}/core_platform/cmake/toolchain/arm-none-eabi-gcc.cmake -DTARGET_CPU=cortex-m55 -DBUILD_CMSIS_NN_FUNCTIONS=YES -DCMSIS_PATH=${CMSIS_PATH}
+
 mkdir ${CMSIS_PATH}/CMSIS/NN/build/ && cd ${CMSIS_PATH}/CMSIS/NN/build/
 cmake .. -DCMAKE_TOOLCHAIN_FILE=${ethosu_dir}/core_platform/cmake/toolchain/arm-none-eabi-gcc.cmake -DTARGET_CPU=cortex-m55 -DBUILD_CMSIS_NN_FUNCTIONS=YES
 make


Both of the above commands look similar, I hope they are serving two different things. Can you please revisit them?

The first one builds the new CMSIS-NN, and the second one builds the old CMSIS/NN.
When the docker image will be updated I'll remove support for CMSIS/NN, that is, I''ll revert this commit: ff57097.

ashutosh-arm · 2023-01-09T17:02:57Z

tests/micro/zephyr/test_zephyr.py

@@ -623,7 +624,7 @@ def test_schedule_build_with_cmsis_dependency(workspace_dir, board, microtvm_deb
    assert "CMSIS/DSP/Include" in cmake_content
    assert "CMSIS/DSP/Include/dsp" in cmake_content
    assert "CMSIS/DSP/Include" in cmake_content
-    assert "CMSIS/NN/Include" in cmake_content
+    # assert "CMSIS-NN/Include" in cmake_content


If comment is not useful for future, it can be removed?

In the future, after the docker image will be updated, we want this assert.

ashutosh-arm · 2023-01-09T17:05:29Z

tests/python/relay/aot/corstone300.mk

+	CMSIS_NN_PATH = ${CMSIS_PATH}/CMSIS-NN
+else
+	CMSIS_NN_PATH = ${CMSIS_PATH}/CMSIS/NN
+endif


doesn't simply looking for the directory work if [ -d " ${CMSIS_PATH}/CMSIS-NN" ]; for simplicity - just curious?

If you want I can check and improve that, but this code will be reverted when the docker image will be updated.

mehrdadh · 2023-01-09T19:13:00Z

apps/microtvm/zephyr/template_project/CMakeLists.txt.template

@@ -31,21 +31,27 @@ find_package(Zephyr HINTS $ENV{ZEPHYR_BASE})
 project(microtvm_autogenerated_project)

 if(DEFINED CMSIS_PATH)
+  if (EXISTS ${CMSIS_PATH}/CMSIS-NN)


Is this a temporary fix until we update the docker image?

mehrdadh · 2023-01-09T19:29:49Z

It will be hard to keep track of all the follow up changes that are required after we update the image. Could you please add comment in all of those places so we can track them?

NicolaLancellotti · 2023-01-09T22:24:23Z

It will be hard to keep track of all the follow up changes that are required after we update the image. Could you please add comment in all of those places so we can track them?

Hi @mehrdadh, thanks for the review.
I wrote the commits in this pr in such a way that it is easy to track the required changes, we just have to revert this commit ff57097 when the image will be updated.

ashutosh-arm · 2023-01-10T17:06:04Z

After discussing offline about the remaining issues with @NicolaLancellotti, it looks like the support for the old CMSIS-NN location will be removed in future. Overall the changes LGTM! Thanks @NicolaLancellotti for this critical update.

mehrdadh

LGTM, thanks!

Pr: apache#13656 adds support for the new CMSIS NN project Change-Id: I28e99fd511b42d9a77ebc6bdc085fc5ffc6b533a

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.

CMSIS NN has been moved out of the CMSIS project into a new GitHub project. This pr adds support for CMSIS NN from this new GitHub location. Both CMSIS and CMSIS NN are now downloaded in /opt/arm/ethosu/cmsis I updated the pr with three commits: - First, I supported CMSIS NN from the new GitHub location, as previously done. - Second, I prevented the definition of the `cmsis_path` project API option from an environment variable. Before, thanks to the environment variable, the `cmsis_path` option was always enabled. That was not a problem, but now CMSIS NN uses a new header (`arm_acle.h`) which is not always present, so we need to explicitly enable CMSIS when we need it. - In the end, I re-added support for the old location of CMSIS NN because the docker image is not yet updated, and we need the tests to pass to accept this pr. In this way, tvm will use the new CMSIS NN project when we will update the docker image, but for now, it uses the old one. I'll create a pr that reverts this last commit when the docker image is updated.

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.

mehrdadh requested changes Jan 5, 2023

View reviewed changes

NicolaLancellotti force-pushed the cmsis-nn/github-location branch from bdc555b to 359efd3 Compare January 9, 2023 13:28

NicolaLancellotti added 3 commits January 9, 2023 16:45

[CMSIS-NN] Support CMSIS NN from new GitHub location

e928a25

Change-Id: I2c39bc671a8a2583f8e271cae98f61f8cabd1ab9

Prevent definition of cmsis_path from env variable

aefe78e

Change-Id: I1585ec3b31626bde5fa5c70224b1c70b48ceeadf

Add support for the old CMSIS NN location

ff57097

Change-Id: If6237cc6903d03d0e3ff04e849a6812f1980244a

NicolaLancellotti force-pushed the cmsis-nn/github-location branch from 359efd3 to ff57097 Compare January 9, 2023 16:51

ashutosh-arm reviewed Jan 9, 2023

View reviewed changes

mehrdadh reviewed Jan 9, 2023

View reviewed changes

mehrdadh approved these changes Jan 10, 2023

View reviewed changes

mehrdadh merged commit 746fcaa into apache:main Jan 10, 2023

NicolaLancellotti added a commit to NicolaLancellotti/tvm that referenced this pull request Jan 11, 2023

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

af54251

Pr: apache#13656 adds support for the new CMSIS NN project Change-Id: I28e99fd511b42d9a77ebc6bdc085fc5ffc6b533a

This was referenced Jan 11, 2023

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

Merged

Remove tutorials CMSIS dependency when not needed #13762

Merged

NicolaLancellotti added a commit to NicolaLancellotti/tvm that referenced this pull request Jan 17, 2023

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

c7be7cb

Pr: apache#13656 adds support for the new CMSIS NN project Change-Id: I28e99fd511b42d9a77ebc6bdc085fc5ffc6b533a

mehrdadh pushed a commit that referenced this pull request Jan 17, 2023

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

c9b4016

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.

ysh329 mentioned this pull request Apr 17, 2023

[Release] v0.12.0 Release Candidate Notes #14645

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CMSIS-NN] Support CMSIS NN from new GitHub location #13656

[CMSIS-NN] Support CMSIS NN from new GitHub location #13656

NicolaLancellotti commented Dec 23, 2022 •

edited

Loading

tvm-bot commented Dec 23, 2022

ashutosh-arm commented Dec 23, 2022

NicolaLancellotti commented Dec 23, 2022

mehrdadh left a comment

NicolaLancellotti commented Jan 9, 2023

ashutosh-arm Jan 9, 2023

NicolaLancellotti Jan 9, 2023

ashutosh-arm Jan 9, 2023

NicolaLancellotti Jan 9, 2023

ashutosh-arm Jan 9, 2023

NicolaLancellotti Jan 9, 2023

mehrdadh Jan 9, 2023

NicolaLancellotti Jan 9, 2023

mehrdadh commented Jan 9, 2023

NicolaLancellotti commented Jan 9, 2023

ashutosh-arm commented Jan 10, 2023

mehrdadh left a comment

[CMSIS-NN] Support CMSIS NN from new GitHub location #13656

[CMSIS-NN] Support CMSIS NN from new GitHub location #13656

Conversation

NicolaLancellotti commented Dec 23, 2022 • edited Loading

tvm-bot commented Dec 23, 2022

ashutosh-arm commented Dec 23, 2022

NicolaLancellotti commented Dec 23, 2022

mehrdadh left a comment

Choose a reason for hiding this comment

NicolaLancellotti commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehrdadh commented Jan 9, 2023

NicolaLancellotti commented Jan 9, 2023

ashutosh-arm commented Jan 10, 2023

mehrdadh left a comment

Choose a reason for hiding this comment

NicolaLancellotti commented Dec 23, 2022 •

edited

Loading