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

Update Ethos-U driver. #2626

Merged
merged 4 commits into from
Jul 24, 2024
Merged

Conversation

iabdalkader
Copy link
Contributor

@iabdalkader iabdalkader commented Jul 14, 2024

Note I tested with TARGET=cortex_m_corstone_300 test_network_tester_test and on real hardware as well built with:

make -j12 -f tensorflow/lite/micro/tools/make/Makefile \
TARGET=cortex_m_generic TARGET_ARCH=cortex-m55 \
CO_PROCESSOR=ethos_u ETHOSU_ARCH=u55 OPTIMIZED_KERNEL_DIR=ethos_u  \
CORE_OPTIMIZATION_LEVEL=-O2 KERNEL_OPTIMIZATION_LEVEL=-O2 \
THIRD_PARTY_KERNEL_OPTIMIZATION_LEVEL=-O2 \
TARGET_TOOLCHAIN_ROOT=/opt/arm-none-eabi/bin/ \ 
TARGET_TOOLCHAIN_PREFIX=arm-none-eabi- BUILD_TYPE=release microlite

BUG=#2619

@iabdalkader iabdalkader requested a review from a team as a code owner July 14, 2024 11:08
Copy link

google-cla bot commented Jul 14, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@iabdalkader iabdalkader force-pushed the ethos_u_driver_update branch 3 times, most recently from 562e3f7 to 730c57f Compare July 16, 2024 09:20
@iabdalkader
Copy link
Contributor Author

Rebased.. unsure how to fix that CI entry test error.

@@ -40,5 +40,3 @@ EMBARC_MLI_PRE_COMPILED_URL := "https://github.com/foss-for-synopsys-dwc-arc-pro
EMBARC_MLI_PRE_COMPILED_MD5 := "173990c2dde4efef6a2c95b92d1f0244"

# Skip md5sum-check since ethos-u-core-driver download link is non-deterministic, see https://github.com/google/gitiles/issues/84
ETHOSU_URL := "https://review.mlplatform.org/plugins/gitiles/ml/ethos-u/ethos-u-core-driver/+archive/24455eedb9e8939f8a28ca0101a6f2d171e1b2f9.tar.gz"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you have a particular reason for migrating from using a download link to using git clone? The URL approach would continue to work by simply updating the link to:

https://review.mlplatform.org/plugins/gitiles/ml/ethos-u/ethos-u-core-driver/+archive/9622608a5cc318c0933bcce720b59737d03bfb6f.tar.gz

Regardless of HTTP or git download, it is important that we version based on the SHA, not the tag. Tags can be moved, which can result in our scripts breaking (this has happened before). While SHAs can technically be erased too, that is significantly less likely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you have a particular reason for migrating from using a download link to using git clone?

I was just following the existing pattern for the other Ethos platform package, which is also a git repo. It's also easier to tell which specific version of the driver is being used from a tag or SHA than from a download URL or even the downloaded package. Finally, probably only important for development, but if you want to check out a different commit for testing something, you can't do that with a downloaded package.

I've switched to commit SHA and retested the build, and retested on hardware.

@rascani
Copy link
Collaborator

rascani commented Jul 16, 2024

Rebased.. unsure how to fix that CI entry test error.

Don't worry about this, it's just an issue I haven't had time to fix in the CI system. Basically, it is erroring because it doesn't have the ready to merge label applied, but that shouldn't be needed until after the PR is approved.

Thank you for the PR! It is greatly appreciated.

@mergify mergify bot merged commit 213eb87 into tensorflow:main Jul 24, 2024
46 of 47 checks passed
@iabdalkader iabdalkader deleted the ethos_u_driver_update branch July 24, 2024 23:31
@iabdalkader
Copy link
Contributor Author

@rascani Seems my patch might have broken some test here: https://github.com/tensorflow/tflite-micro/actions/runs/10087978372/job/27892985767

I'll try to fix it.

iabdalkader added a commit to iabdalkader/tflite-micro that referenced this pull request Jul 25, 2024
The updated Ethos-U driver v24.05 (introduced in tensorflow#2626), contains a second
set of device functions for the U85. This can cause the linker to link the
functions of the U85, instead of the U55-U65, if they're both included in
the driver's sources files. Therefore a glob pattern can no longer be used
to collect the driver's source files in the Makefile.
iabdalkader added a commit to iabdalkader/tflite-micro that referenced this pull request Jul 25, 2024
The updated Ethos-U driver v24.05 (introduced in tensorflow#2626), contains
a second set of device functions for the U85. This can cause the linker
to link the functions of the U85, instead of the U55-U65, if they're both
included in the driver's sources files. Therefore a glob pattern can no
longer be used to collect the driver's source files in the Makefile.
@rascani
Copy link
Collaborator

rascani commented Jul 25, 2024

Thanks @iabdalkader

mergify bot pushed a commit that referenced this pull request Jul 25, 2024
The updated Ethos-U driver v24.05 (introduced in #2626), contains a second set of device functions for the U85. This can cause the linker to link the functions of the U85, instead of the U55-U65, if they're both included in the driver's sources files. Therefore a glob pattern can no longer be used to collect the driver's source files in the Makefile.

BUG=#2639
iabdalkader added a commit to iabdalkader/tflite-micro that referenced this pull request Jul 26, 2024
* Update Ethos-U driver to the latest (v24.05)
* Allow passing extra C flags to the driver from the command line.
* Fixes tensorflow#2619

Note I tested with `TARGET=cortex_m_corstone_300 test_network_tester_test` and on real hardware as well built with:
```bash
make -j12 -f tensorflow/lite/micro/tools/make/Makefile \
TARGET=cortex_m_generic TARGET_ARCH=cortex-m55 \
CO_PROCESSOR=ethos_u ETHOSU_ARCH=u55 OPTIMIZED_KERNEL_DIR=ethos_u  \
CORE_OPTIMIZATION_LEVEL=-O2 KERNEL_OPTIMIZATION_LEVEL=-O2 \
THIRD_PARTY_KERNEL_OPTIMIZATION_LEVEL=-O2 \
TARGET_TOOLCHAIN_ROOT=/opt/arm-none-eabi/bin/ \ 
TARGET_TOOLCHAIN_PREFIX=arm-none-eabi- BUILD_TYPE=release microlite
```

BUG=tensorflow#2619
iabdalkader added a commit to iabdalkader/tflite-micro that referenced this pull request Jul 26, 2024
The updated Ethos-U driver v24.05 (introduced in tensorflow#2626), contains a second set of device functions for the U85. This can cause the linker to link the functions of the U85, instead of the U55-U65, if they're both included in the driver's sources files. Therefore a glob pattern can no longer be used to collect the driver's source files in the Makefile.

BUG=tensorflow#2639
iabdalkader added a commit to iabdalkader/tflite-micro that referenced this pull request Jul 26, 2024
* Update Ethos-U driver to the latest (v24.05)
* Allow passing extra C flags to the driver from the command line.
* Fixes tensorflow#2619

Note I tested with `TARGET=cortex_m_corstone_300 test_network_tester_test` and on real hardware as well built with:
```bash
make -j12 -f tensorflow/lite/micro/tools/make/Makefile \
TARGET=cortex_m_generic TARGET_ARCH=cortex-m55 \
CO_PROCESSOR=ethos_u ETHOSU_ARCH=u55 OPTIMIZED_KERNEL_DIR=ethos_u  \
CORE_OPTIMIZATION_LEVEL=-O2 KERNEL_OPTIMIZATION_LEVEL=-O2 \
THIRD_PARTY_KERNEL_OPTIMIZATION_LEVEL=-O2 \
TARGET_TOOLCHAIN_ROOT=/opt/arm-none-eabi/bin/ \ 
TARGET_TOOLCHAIN_PREFIX=arm-none-eabi- BUILD_TYPE=release microlite
```

BUG=tensorflow#2619
iabdalkader added a commit to iabdalkader/tflite-micro that referenced this pull request Jul 26, 2024
The updated Ethos-U driver v24.05 (introduced in tensorflow#2626), contains a second set of device functions for the U85. This can cause the linker to link the functions of the U85, instead of the U55-U65, if they're both included in the driver's sources files. Therefore a glob pattern can no longer be used to collect the driver's source files in the Makefile.

BUG=tensorflow#2639
iabdalkader added a commit to iabdalkader/tflite-micro that referenced this pull request Jul 26, 2024
The updated Ethos-U driver v24.05 (introduced in tensorflow#2626), contains a second set of device functions for the U85. This can cause the linker to link the functions of the U85, instead of the U55-U65, if they're both included in the driver's sources files. Therefore a glob pattern can no longer be used to collect the driver's source files in the Makefile.

BUG=tensorflow#2639
@mansnils
Copy link
Contributor

Thanks for this @iabdalkader

@iabdalkader
Copy link
Contributor Author

You're welcome. Note that there were some issue that I've missed because the workflow runs on schedule. The workflow for Cortex-M doesn't take much time, just a little over 10 minutes, so perhaps it's best to change it to trigger on PRs. This could save a lot of time in the future.

@mansnils
Copy link
Contributor

Yes agreed. We are looking into that.

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.

Feature request: Update Ethos-U driver ?
4 participants