Enable RISC-V cross build and test #1477
Conversation
tools/riscv/README_RISCV.md
Outdated
| cmake --build ./build-riscv --target install | ||
| ``` | ||
|
|
||
| ## Enable RISC-V Extensions at Runtime ## |
There was a problem hiding this comment.
We don't have enabling of extensions at runtime for any other architecture. Perhaps this should be done in a different PR for all architectures.
There was a problem hiding this comment.
I apologize for using a misleading name. The feature is designed to get risc-v cpu info, because we can't use hwcap to detect risc-v extensions at runtime in linux. We may find better solution in the future.
There was a problem hiding this comment.
Enabling auto-vectorization in compiler and run-time checking of vector instruction support are kind of mutually exclusive, so I would like to see manually (optimized by hand) vectorized functions along with run-time checking.
There was a problem hiding this comment.
Sure, the USE_AUTO_VECTORIZER option is turned off by default, but users can still enable it, if they know what they are doing.
zlib-ng/cmake/toolchain-riscv.cmake
Line 6 in 9151067
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## develop #1477 +/- ##
===========================================
+ Coverage 83.15% 83.20% +0.04%
===========================================
Files 130 130
Lines 10765 10761 -4
Branches 2765 2767 +2
===========================================
+ Hits 8952 8954 +2
+ Misses 1139 1133 -6
Partials 674 674
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
|
This looks good for the most part, but there are two things I am not happy about being introduced. I really hope it is possible to work around this somehow, although that might need hwcap updates to the kernel if I understand correctly? An alternative is of course to force enable/disable this during compile, that would move the burden onto packagers instead, they could for example compile for multiple variations and only select the correct one during install. For testing or manual compile one could compile for the correct variation directly. |
I believe that it's reasonable to determine RVV supporting during compile time, and to implement a more graceful solution when the kernel updates hwcaps or hwprobe for risc-v in the features. |
"Force enable" during configuring or compiling is a lot better compromise than relying on user input being sane or some kernel provided text file keeping predictable formatting. |
cmake/toolchain-riscv.cmake
Outdated
| set(CMAKE_C_FLAGS "${RISCV_COMPILER_FLAGS} ${CMAKE_C_FLAGS}") | ||
| set(CMAKE_CXX_FLAGS "${RISCV_COMPILER_FLAGS} ${CMAKE_CXX_FLAGS}") | ||
|
|
||
| set(RISCV_LINKER_FLAGS "-lstdc++ -lpthread -lm -ldl") |
There was a problem hiding this comment.
Unconditionally adding -lstdc++ and -lpthread might need a comment to explain the reason... zlib-ng library itself is C code, so shouldn't link against C++ standard library.
Usually used C++ library is specified in CMAKE_CXX_FLAGS_INIT using -stdlib=libc++or -stdlib=libstdc++ before setting CMAKE_CXX_FLAGS when there is multiple choices.
There was a problem hiding this comment.
Thanks, we should allow users to choose the options based on their conditions.
Thanks, it's done. |
|
There is clearly issue with building the dependencies taking too long. It would be a lot easier if there would be official deb packages for the dependencies instead of needing to rebuild them every single time. |
Thank you for your feedback. It is indeed a big issue, especially for modern platforms like RISC-V, which has not yet been widely used in the world. We are working on solving this problem, and I will update the message thread with any progress. |
|
Getting qemu and other packages into the repos is the best solution, but that will take time. |
Docker is just wrapper around In special cases it might be enough to compile the binaries once, use |
|
Docker does not use qemu emulation to run applications on their native architecture. A docker can be just that, a tar download containing a prebuilt environment and any scripts to set things up correctly. |
| @@ -720,6 +725,24 @@ if(WITH_OPTIM) | |||
| set(WITH_POWER9 OFF) | |||
| endif() | |||
| endif() | |||
| elseif(BASEARCH_RISCV_FOUND) | |||
| if(WITH_RVV) | |||
There was a problem hiding this comment.
There should be check_rvv_intrinics function that checks if the compiler supports -march=rv64gcv. And then that flag can be removed from the toolchain file.
There was a problem hiding this comment.
Perhaps the next line check_c_source_compiles should be moved into a check_rvv_intrinics with the CMAKE_REQUIRED_FLAGS = -march=rv64gcv
There was a problem hiding this comment.
Thanks, it's done. We will use set_property to add compiler flags "-march=rv64gcv" to the files for rvv optimized kernel in the related pr.
tools/riscv/README_RISCV.md
Outdated
| cmake --build ./build-riscv | ||
| ``` | ||
|
|
||
| Disable the option if there is no RVV support: |
There was a problem hiding this comment.
CMake should determine if the compiler supports RVV. WITH_RVV should only be there to person compiling it, to be able to easily enable/disable the feature.
There was a problem hiding this comment.
Because we cannot detect rvv support at runtime, running the rvv code on a no-rvv target is a risk. Users could disable the rvv when the target don't support rvv.
We will have a better solution when the kernel updates hwcap or hwprobe for riscv.
There was a problem hiding this comment.
Perhaps at the top of this readme, below, Building RISC-V Target with Cmake it should indicate that there is no support for runtime detection atm.
Thank you for your suggestions. I have created an automated release workflow for the prebuilt toolchain. Users can now download the prebuilt toolchain and enjoy using it. Users can also use the script to download and build the toolchain based on their environment. Apologies for the Docker plan, we cannot provide the RISC-V Docker in the short term. However, I will continue working on implementing this feature. |
|
Actually you can use GitHub CLI to download GH releases. https://cli.github.com/manual/gh_release_download GH cli is already installed on all GH runners. |
|
The RISC-V toolchain doesn't seem to support coverage... |
cmake/detect-intrinsics.cmake
Outdated
| @@ -347,6 +347,19 @@ macro(check_power8_intrinsics) | |||
| set(CMAKE_REQUIRED_FLAGS) | |||
| endmacro() | |||
|
|
|||
| macro(check_rvv_intrinsics) | |||
| # Check whether compiler supports RVV | |||
| set(CMAKE_REQUIRED_FLAGS "-march=rv64gcv") | |||
There was a problem hiding this comment.
This should be similar to check_power9_intrinsics where it would do:
if(CMAKE_C_COMPILER_ID MATCHES "GNU" OR CMAKE_C_COMPILER_ID MATCHES "Clang")
if(NOT NATIVEFLAG)
set(RVVFLAG "-march=rv64gcv")
endif()
endif()
# Check if we have what we need for RISCV optimizations
set(CMAKE_REQUIRED_FLAGS "${RVVFLAG} ${NATIVEFLAG}")
...There was a problem hiding this comment.
Thanks, it's done.
cmake/toolchain-riscv.cmake
Outdated
| set(CMAKE_OBJCOPY "${TOOLCHAIN_PATH}/bin/llvm-objcopy") | ||
|
|
||
| # compile options | ||
| set(RISCV_COMPILER_FLAGS "" CACHE STRING "The riscv compiler flags.") |
There was a problem hiding this comment.
Thanks, the cmake file was over-designed, let’s remove the unused parts.
cmake/toolchain-riscv.cmake
Outdated
| # disable auto-vectorizer | ||
| add_compile_options(-fno-vectorize -fno-slp-vectorize) | ||
|
|
||
| set(CMAKE_C_FLAGS "${RISCV_COMPILER_FLAGS} ${CMAKE_C_FLAGS}") |
cmake/toolchain-riscv.cmake
Outdated
| set(CMAKE_CXX_FLAGS "${RISCV_COMPILER_FLAGS} ${CMAKE_CXX_FLAGS}") | ||
|
|
||
| # link options | ||
| set(RISCV_LINKER_FLAGS "" CACHE STRING "The riscv specific link options.") |
There was a problem hiding this comment.
No need for thse linkers flags either? Let's not do them until they are used...
tools/riscv/README_RISCV.md
Outdated
| cmake --build ./build-riscv | ||
| ``` | ||
|
|
||
| Disable the option if there is no RVV support: |
There was a problem hiding this comment.
Perhaps at the top of this readme, below, Building RISC-V Target with Cmake it should indicate that there is no support for runtime detection atm.
Do you mean the codecov report? |
I mean generating coverage information during compilation... First step of cmake gives warning that the relevant compiler command-line options are not supported. |
tools/riscv/README_RISCV.md
Outdated
| @@ -0,0 +1,45 @@ | |||
| # Building RISC-V Target with Cmake # | |||
There was a problem hiding this comment.
Can you rename tools/riscv/README_RISCV.md to arch/riscv/README.md similar to s390?
There was a problem hiding this comment.
Sure, it's done.
|
This currently fails CI due to a missing github access token. Strangely the downloader doesn't fall back to just using the default access token created for each CI job. https://github.com/zlib-ng/zlib-ng/actions/runs/4930653399/jobs/8855048977?pr=1477 According to this: https://docs.github.com/en/actions/security-guides/automatic-token-authentication |
Add RISC-V cross-compilation test Enable RVV support at compile time
Thanks, it's done. |
Add supporting RISC-V cross compilation workflows.
Add riscv cpu info detection.