davsva01
commented
1 year ago Hi @ilya-lavrenov, thank you for your feedback!
To supply some context the initial focus for the CMake build was created as part of integrating ComputeLibrary into PyTorch. This build targets running PyTorch with ComputeLibrary on Neoverse targets. This means delivering a limited build that does not support the full range of options available in the scons build. This implementation was made to be similar to the multi_isa build in scons. This is the reason for decisions such as building with SVE support, since this allows us to run the compiled code on both SVE and non-SVE hardware. We hope to be able to expand the capabilities of the CMake (and Bazel) build options over time, and we’d welcome any contributions.
Cmake options are named too generic
That is very fair, we will implement this for a future release.
Some options do not have proper effect on code building. E.g. on macOS SVE is not supported, so I expected that if I turn all SVE-related options OFF, then build will pass. But unfortunately, all SVE libraries are added unconditionally
In this initial build we’ve only targeted Linux native builds. This is a requirement of our initial build but could be relaxed to allow builds for a wider variety of systems.
The same issue with OpenMP: we don't want to use OpenMP and want to rely on cppthreads. While, OpenMP scheduler is added to sources unconditionally
That’s a good point, we will make the inclusion of the scheduler files optional depending on build.
Scons has an option to specify architecture we want to build for. Why cmake interface does not provide such option and always builds for fp16? how can we build for 32bits ARM platforms?
The initial build targets Neoverse targets which is why fp16 is always enabled in this build. This is something we plan to address in a future patch though.
Strange decision to fail with fatal error because of compiler version
This is due to the target build requiring SVE (arm_sve.h) in our case. This can be relaxed and expanded to a wider set of builds.
Instead of having DEBUG option
That’s fair as well, we will look into this for a future release.
Add an option to disable tests building
Absolutely, we will look into making this optional as well.
What's about OpenCL? Is it disabled in cmake?
OpenCL is currently not considered in the build due to targeting Neoverse only for now.
Hi,
It's great that ARM Compute Library has provided cmake interface to build the project. As far as I can understand, it's kind of initial implementation, so I want to provide the feedback:
Cmake options are named too generic: https://github.com/ARM-software/ComputeLibrary/blob/d8bf9b53752a4f573120cf51b31055de8b3c7d29/cmake/Options.cmake#L29-L39 It means if the project is used as subproject in a bigger project, name clashes can happen. It's a good practice to have a common prefix for all the options, e.g.
ARM_COMPUTE_Some options do not have proper effect on code building. E.g. on macOS SVE is not supported, so I expected that if I turn all SVE-related options OFF, then build will pass. But unfortunately, all SVE libraries are added unconditionally: https://github.com/ARM-software/ComputeLibrary/blob/d8bf9b53752a4f573120cf51b31055de8b3c7d29/CMakeLists.txt#L205-L207
The same issue with OpenMP: we don't want to use OpenMP and want to rely on cppthreads. While, OpenMP scheduler is added to sources unconditionally: https://github.com/ARM-software/ComputeLibrary/blob/d8bf9b53752a4f573120cf51b31055de8b3c7d29/src/CMakeLists.txt#L950
Scons has an option to specify architecture we want to build for. Why cmake interface does not provide such option and always builds for fp16? how can we build for 32bits ARM platforms? https://github.com/ARM-software/ComputeLibrary/blob/d8bf9b53752a4f573120cf51b31055de8b3c7d29/CMakeLists.txt#L190
Strange decision to fail with fatal error because of compiler version: https://github.com/ARM-software/ComputeLibrary/blob/d8bf9b53752a4f573120cf51b31055de8b3c7d29/CMakeLists.txt#L50-L55 We use compilers from debian-9 systems which are quite old, but they perfectly compile code for 32bits and 64bits platforms. Could you please relax such checks? If you want to highlight that it's better to use newer compiler versions, you can warn users about it instead of refusing to continue. Cmake is great because it provides an ability to use multiple systems / toolchains and your project should not break this rule.
Instead of having DEBUG option: https://github.com/ARM-software/ComputeLibrary/blob/d8bf9b53752a4f573120cf51b31055de8b3c7d29/cmake/Options.cmake#L29 put all debug flags to
CMAKE_CXX_FLAGS_DEBUG. In this case cmake automatically will add flags depending on desired build configuration. This is required, because modern build systems like Ninja, MSVC allows to generate project once and use it for Debug / Release. So, setting build flags on the project generation stage will break these build systems.Add an option to disable tests building: https://github.com/ARM-software/ComputeLibrary/blob/d8bf9b53752a4f573120cf51b31055de8b3c7d29/CMakeLists.txt#L243 Tests are required for developers (for you), but not for users (we are as open source users). So, having tests disabled by default is also a good practice.
What's about OpenCL? Is it disabled in cmake?