-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[ComputeLibrary] Add new package for ARM-software/ComputeLibrary #19958
[ComputeLibrary] Add new package for ARM-software/ComputeLibrary #19958
Conversation
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 7d86026compute_library/23.08@#557d1db9635cd53cff3ff429a550ed93
|
yes_no = lambda v: "1" if v else "0" | ||
debug = yes_no(self.settings.build_type == "Debug") | ||
build_os = str(self.settings.os).lower() | ||
arch = {"armv8": "armv8a", "x86": "x86_32", "armv7": "armv7a", "armv7hf": "armv7a-hf"}.get(str(self.settings.arch), str(self.settings.arch)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ARM compute allows to build code with armv8.2a (fp16 support), SVE and SME.
How to use it via this recipe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for example Apple M1, M2 support fp16, so we need to enable it by default.
minimum_version = self._compilers_minimum_version.get(str(self.settings.compiler), False) | ||
if minimum_version and Version(self.settings.compiler.version) < minimum_version: | ||
raise ConanInvalidConfiguration(f"{self.ref} requires C++{self._min_cppstd}, which your compiler does not support.") | ||
supported_os = ["Android", "Linux", "OpenBSD", "Macos", "Tizen"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but it's not documented by the upstream: https://github.com/ARM-software/ComputeLibrary#supported-systems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# INFO: Using scons to build the library we don't have control over shared/static and install step, it is done all together always | ||
# INFO: https://arm-software.github.io/ComputeLibrary/latest/how_to_build.xhtml | ||
yes_no = lambda v: "1" if v else "0" | ||
debug = yes_no(self.settings.build_type == "Debug") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can also add these options for debug debug=1 asserts=1 logging=1
if str(self.settings.os) not in supported_os: | ||
raise ConanInvalidConfiguration(f"{self.ref} does not support {self.settings.os}. It is only supported on {supported_os}.") | ||
if "arm" not in str(self.settings.arch) and "x86" not in str(self.settings.arch): | ||
raise ConanInvalidConfiguration(f"{self.ref} does not support {self.settings.arch}. It is only supported on arm and x86.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arm and x86 usually imply 32bits
should we write arm / arm64 and x86_64 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good clarification.
minimum_version = self._compilers_minimum_version.get(str(self.settings.compiler), False) | ||
if minimum_version and Version(self.settings.compiler.version) < minimum_version: | ||
raise ConanInvalidConfiguration(f"{self.ref} requires C++{self._min_cppstd}, which your compiler does not support.") | ||
supported_os = ["Android", "Linux", "OpenBSD", "Macos", "Tizen"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but it's not documented by the upstream: https://github.com/ARM-software/ComputeLibrary#supported-systems
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 158d0c7compute_library/23.08@#bbfb9834bb59b8d05cadb58204eb4f74
|
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit e3e2f36compute_library/23.08@#77aaf6972aa3a2e9d1856df00349d77e
|
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
if "armv8" not in str(self.settings.arch) and self.options.enable_multi_isa: | ||
raise ConanInvalidConfiguration(f"{self.ref} does not support multi_isa option for {self.settings.arch}. It is only supported on armv8.") | ||
if self.settings.arch == "armv8" and self.build_settings.arch == "x86_64" and self.settings.os == "Macos": | ||
raise ConanInvalidConfiguration(f"Mac Intel is not supported for armv8-a. Please, use Mac M1 as native build.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, cross compilation from macOS arm64 to x86_64 (as vice versa) works well. You need to use -arch <arm64 | x86_64>
option.
See https://github.com/openvinotoolkit/openvino/blob/master/src/plugins/intel_cpu/thirdparty/ACLConfig.cmake#L285-L288 as example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is: I'm using scons and it changes to armv8-a
that's not identified by apple clang and passing as -march
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also use scons, but it is just run from cmake.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit ff185c2compute_library/23.02.1@#47dafd2c82db97de0174fe779d93eb19
compute_library/23.08@#418fdff5101d761b59739ceaf986b2c5
|
multi_isa = yes_no(self.options.enable_multi_isa) | ||
build = "cross_compile" if cross_building(self) else "native" | ||
with chdir(self, self.source_folder): | ||
self.run(f"scons Werror=0 validation_tests=0 examples=0 gemm_tuner=0 multi_isa={multi_isa} openmp={openmp} debug={debug} neon={neon} opencl={opencl} os={build_os} arch={arch} build={build} build_dir={self.build_folder} install_dir={self.package_folder} -j{build_jobs(self)} toolchain_prefix=''", env="conanbuild") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conan's arch and compute library archs are different. Compute Library allows to specify exact optimizations which are required. Without it, it does not give proper performance.
So, bypassing conan's arch to compute library is not enough. Conan's arch can be used as initial guess, while users still need to have an ability to provide an option for arch to enable specific optimizations like SVE, SME
As I wrote - Apple M1 must set armv8.2-a to enable FP16 optimization. Currently, it's not set and performance is poor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, you can customize your settings.yml, providing not only new archs, but also new subsettings: https://docs.conan.io/2/reference/config_files/settings.html#customizing-settings
Conan covers most generic and known archs, but when talking about optimization, is better having a specific profile adapted, so you can be sure about what's in your package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with such approach.
If we put it on x64 arch, what I am asking is to provide a way to select between SSE, AVX, AMX optimizations, while you want me to customize whole settings file and add SSE, AVX and so on to be able to properly compile the library. Don't mix different platforms (settings) against different library optimizations capabilities (the arm
option of ARM Compute Library).
Any reason why on Apple ARM64 people would not have FP16 support (which is available out of box) and have to change settings to get suitable performance?
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Conan v1 pipeline ✔️All green in build 8 (
Conan v2 pipeline ✔️
All green in build 8 ( |
Hooks produced the following warnings for commit 8064b14compute_library/23.02.1@#f6f4e87eeace0a9e27d5728842898b34
compute_library/23.08@#552c581b77110a37876484b05ed29789
|
if str(self.settings.os) not in supported_os: | ||
raise ConanInvalidConfiguration(f"{self.ref} does not support {self.settings.os}. It is only supported on {supported_os}.") | ||
if "arm" not in str(self.settings.arch) and "x86" not in str(self.settings.arch): | ||
raise ConanInvalidConfiguration(f"{self.ref} does not support {self.settings.arch}. It is only supported on arm and x86.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good clarification.
Specify library name and version: compute_library/23.08
That's a requirement for #19681 (OpenVino)
The ComputeLibrary has experimental CMake support, but it's restricted to a single linux profile, so it does not work with Mac. The official documentation uses SCons: https://github.com/ARM-software/ComputeLibrary