Skip to content

Added support to build onnxruntime with ACL#3586

Merged
prabhat00155 merged 3 commits intomicrosoft:masterfrom
prabhat00155:prroy/acl_changes
Apr 20, 2020
Merged

Added support to build onnxruntime with ACL#3586
prabhat00155 merged 3 commits intomicrosoft:masterfrom
prabhat00155:prroy/acl_changes

Conversation

@prabhat00155
Copy link
Contributor

Description: With these changes, onnxruntime could be built with ACL ep using:
./build.sh --use_acl

Motivation and Context

  • Why is this change required? What problem does it solve? This adds ACL ep support to onnxruntime.
  • If it fixes an open issue, please link to the issue here.

@prabhat00155 prabhat00155 requested a review from a team April 18, 2020 13:16
1. Build ACL Library (skip if already built)
```
cd ~
git clone https://github.com/Arm-software/ComputeLibrary.git
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an example for someone who wants to build ACL. If they have a pre-built version, they may use it as mentioned above.

Copy link
Contributor

@HectorSVC HectorSVC left a comment

Choose a reason for hiding this comment

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

:shipit:

source_group(TREE ${ONNXRUNTIME_ROOT}/core FILES ${onnxruntime_providers_acl_cc_srcs})
add_library(onnxruntime_providers_acl ${onnxruntime_providers_acl_cc_srcs})
onnxruntime_add_include_to_target(onnxruntime_providers_acl onnxruntime_common onnxruntime_framework onnx onnx_proto protobuf::libprotobuf)
target_link_libraries(onnxruntime_providers_acl -L$ENV{LD_LIBRARY_PATH})
Copy link
Member

Choose a reason for hiding this comment

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

why using LD_LIBRARY_PATH? that's normally used at run time.

Copy link
Contributor Author

@prabhat00155 prabhat00155 Apr 20, 2020

Choose a reason for hiding this comment

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

We need to link the ACL so files. Without this build fails with this error: "Unable to load arm_compute arm_compute_graph arm_compute_core".

Copy link
Member

Choose a reason for hiding this comment

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

My point was that LD_LIBRARY_PATH should not be used for that purpose.
We should allow user to specify the location of ACL libraries as build option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if you make user pass ACL library location, the user will still need to set LD_LIBRARY_PATH in order to use onnxruntime_perf_test otherwise get this error: "error while loading shared libraries: libarm_compute.so: cannot open shared object file: No such file or directory."

Copy link
Member

@jywu-msft jywu-msft Apr 25, 2020

Choose a reason for hiding this comment

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

yes, but LD_LIBRARY_PATH is used at runtime. it's not meant to be used during build time (which is what the cmake files are for)
at build time, the ACL install location can be anywhere and not set in the user's LD_LIBRARY_PATH
the binary could then be deployed to a target where LD_LIBRARY_PATH would need to be set at run time.
besides, LD_LIBRARY_PATH env variable is specific to Linux/Unix.
Many other EP's have dependency on other libs. (e.g. CUDA, OpenVINO) etc.
But you can grep through all our cmake files and you won't find a single reference to LD_LIBRARY_PATH

@prabhat00155 prabhat00155 merged commit 381fee4 into microsoft:master Apr 20, 2020
//optimized depthwise convolution
#if defined(ACL_1902) || defined(ACL_1905)
auto layer = std::make_shared<arm_compute::NEDepthwiseConvolutionLayer3x3>();
auto layer = std::make_shared<arm_compute::NEDepthwiseConvolutionLayer>();

Choose a reason for hiding this comment

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

What is the reason of this change? NEDepthwiseConvolutionLayer3x3 is an optimized version for depthwise 3x3 which increases the performance significantly for mobilenets and other models.

Copy link
Member

@jywu-msft jywu-msft Apr 22, 2020

Choose a reason for hiding this comment

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

I believe it was accidental.
@prabhat00155 was trying to incorporate some of the changes from #2511
but accidentally reverted changes from
#2774
@prabhat00155 can you please revert this change? thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was getting error(NEDepthwiseConvolutionLayer3x3 not defined) and had to make this change to build. I'll update my ACL repo and retry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got the issue, this should be only used for ACL_1902. I'll create a new PR to use NEDepthwiseConvolutionLayer3x3 and fix the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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