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

[cpu] riscv64: update intrinsics #2929

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zhangfeiv0
Copy link

Description

Currently, oneDNN can be compiled normally on RISC-V architecture platforms, but when using newer compilers and enabling vector extensions, it does not compile RVV-related code (specifically, the code introduced in #1521). Below is the configuration output information regarding RVV when using clang-17:

export CC=clang-17 CXX=clang++-17
cmake .. -DDNNL_CPU_RUNTIME=SEQ -DCMAKE_CXX_FLAGS=-march=rv64gcv -DCMAKE_C_FLAGS=-march=rv64gcv

-- Performing Test CAN_COMPILE_RVV_INTRINSICS
-- Performing Test CAN_COMPILE_RVV_INTRINSICS - Failed
-- Can compile RVV Intrinsics: FALSE
-- DNNL_RISCV_USE_RVV_INTRINSICS: FALSE

This is because the RVV intrinsics in #1521 are based on the old version specifications, and the new versions all have the __riscv64 prefix, as detailed in rvv-intrinsic-doc. Currently, most open-source software projects (such as OpenCV, OpenBLAS, and SLEEF) have already adopted the new RVV intrinsics.

Based on #1521, the following modifications were made:

  1. Updated intrinsics in rvv_nchw_pooling.cpp to the latest version;
  2. Added version checks for intrinsics in CMake.

These changes enable Clang 17+ or GCC 14+ to properly compile RVV-related code, while older compiler versions will fall back to generic implementations.

Checklist

General

  • Do all unit and benchdnn tests (make test and make test_benchdnn_*) pass locally for each commit?

Tested on both QEMU and Banana Pi BPI-F3 (vector length is 256 bits). Due to the long testing time required for matmul, it has been omitted here, as this PR only modifies the names of intrinsics. Below are the test results:

result.log

Signed-off-by: Zhang fei <zhangfei@iscas.ac.cn>
Signed-off-by: Zhang fei <zhangfei@iscas.ac.cn>
Signed-off-by: Zhang fei <zhangfei@iscas.ac.cn>
Signed-off-by: Zhang fei <zhangfei@iscas.ac.cn>
Copy link
Contributor

@mgouicem mgouicem left a comment

Choose a reason for hiding this comment

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

LGTM thanks. Do you know what becomes the minimal compiler version for GCC/clang after this change?

Tagging @pazamelin @aaronfranke @dmitry-gorokhov for awareness.

Signed-off-by: Zhang fei <zhangfei@iscas.ac.cn>
@zhangfeiv0
Copy link
Author

The following is an excerpt from rvv-intrinsic-doc. After the introduction of this PR, the minimum GCC version required will be GCC 14, and the minimum Clang version will be Clang 17.
image

Furthermore, below is the configuration output when compiling OneDNN with different versions of the compiler.

use gcc 13 configuration output iShot_2025-03-21_20 09 55
use gcc 14 configuration output iShot_2025-03-21_20 12 07
use clang 16 configuration output iShot_2025-03-21_20 30 24
use clang 17 configuration output iShot_2025-03-21_20 31 33

@zhangfeiv0 zhangfeiv0 requested a review from mgouicem March 21, 2025 13:06
@aaronfranke
Copy link
Contributor

Thanks for the ping, great to see oneDNN's progress on RISC-V. Note that the vector extension is still not common, so compiling must be supported without it, and that appears to be the case here thankfully. Otherwise it's beyond my expertise to review this.

#endif
#if defined(__riscv_v_intrinsic) && __riscv_v_intrinsic < 12000
#error \"Wrong intrinsics version, v0.12 or higher is required for gcc or clang\"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#error \"Wrong intrinsics version, v0.12 or higher is required for gcc or clang\"
#error \"RISC-V intrinsics v0.12 or higher is required\"

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: Zhang fei <zhangfei@iscas.ac.cn>
@zhangfeiv0 zhangfeiv0 requested a review from vpirogov March 22, 2025 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants