From 01b23a0de5ccc738dc84d5fd2544c3e6f1d6a014 Mon Sep 17 00:00:00 2001 From: kevinylin88 Date: Thu, 21 May 2026 19:29:55 +0800 Subject: [PATCH] Merge pull request #29080 from kevinylin88:project4_kevinlin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit core(rvv): fix v_matmul/v_matmuladd scalable semantics and expand lane-group test coverage #29080 ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work - [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake ## Platform SpacemiT X60 (K1), 8-core RISC-V RVV 1.0, VLEN=256, 16GB RAM, OS: Bianbu Linux (kernel 6.6.63), GCC 13.2.0, Build: OpenCV 4.14.0-pre, Release, HAL: YES (RVV HAL 0.0.1) ## Motivation OpenCV's Universal Intrinsics `v_matmul` and `v_matmuladd` have a semantic bug in the RVV scalable backend (`modules/core/include/opencv2/core/hal/intrin_rvv_scalable.hpp`). The current implementation uses `v_extract_n(v, 0/1/2/3)` with hardcoded indices, assuming the vector holds exactly 4 float lanes (128-bit fixed). On hardware with VLEN=256 (e.g. SpacemiT K1 / BPI-F3), `v_float32` with LMUL=2 holds 16 lanes. As a result, lanes 4–15 silently reuse the inputs from lanes 0–3, producing wrong results. OpenCV itself acknowledges this in `modules/core/src/matmul.simd.hpp`: // v_matmuladd for RVV is 128-bit only but not scalable, // this will fail the test Core_Transform.accuracy The RVV scalable `transform_32f` path has been disabled because of this bug. However, the existing `TheTest::test_matmul()` only checked the first 4-lane group (the outer loop was effectively hardcoded to `int i = 0`), so the bug was never caught by CI even on wide-vector backends. ## Modification **Test fix** (`modules/core/test/test_intrin_utils.hpp`): Expanded `test_matmul()` to iterate over all 4-lane groups: // Before (only checked lane group i=0) int i = 0; for (int j = i; j < i + 4; ++j) { ... } // After (checks all lane groups) for (int i = 0; i < VTraits::vlanes(); i += 4) { for (int j = i; j < i + 4; ++j) { ... } } **Kernel fix** (`modules/core/include/opencv2/core/hal/intrin_rvv_scalable.hpp`): Rewrote `v_matmul` and `v_matmuladd` to process all 4-lane groups correctly. Each group of 4 lanes now independently computes the full matrix multiply using its own `v[i], v[i+1], v[i+2], v[i+3]` inputs. The `transform_32f` RVV path in `matmul.simd.hpp` remains disabled as the autovectorized path shows better performance on current hardware. ## Experiment 1: Bug reproduced on SpacemiT K1 (VLEN=256) ./opencv_test_core --gtest_filter="*intrin*" Result: `hal_intrin128.float32x4_BASELINE` FAILED with 24 failures, all from lane groups i=4, i=8, i=12 (lanes 4–15). Representative failures from `v_matmul` (line 1526): i=4 j=4: actual=158 expected=56 i=4 j=5: actual=166.39999 expected=59.200001 i=8 j=8: actual=314.39999 expected=68.800003 i=12 j=12: actual=512.40002 expected=81.599998 Representative failures from `v_matmuladd` (line 1540): i=4 j=4: actual=147.5 expected=51.5 i=8 j=8: actual=284.70001 expected=60.700001 i=12 j=12: actual=453.89999 expected=69.900002 Lane group i=0 (j=0..3) passed correctly — confirming the bug only affects lanes beyond the first 4, exactly as expected from the hardcoded `v_extract_n(v, 0/1/2/3)` implementation. ## Experiment 2: Both tests pass after fixing the kernel ./opencv_test_core --gtest_filter='hal_intrin128.float32x4_BASELINE' [ OK ] hal_intrin128.float32x4_BASELINE (1859 ms) [ PASSED ] 1 test. ./opencv_test_core --gtest_filter='Core_Transform.accuracy' [ OK ] Core_Transform.accuracy (819 ms) [ PASSED ] 1 test. ## Experiment 3: RVV transform path remains disabled (performance regression) After re-enabling the RVV scalable `transform_32f` path experimentally, benchmarks showed a significant regression vs the compiler-autovectorized scalar path (CV_32FC3): Size RVV path Scalar path Ratio 640x480 7.83 ms 1.48 ms 5.3x slower 1280x720 23.96 ms 5.17 ms 4.6x slower 1920x1080 53.76 ms 10.12 ms 5.3x slower The compiler-autovectorized path outperforms the hand-written RVV kernel for this workload, consistent with the original comment in `matmul.simd.hpp`. The `transform_32f` RVV path is therefore kept disabled in this PR. The kernel fix to `v_matmul`/`v_matmuladd` remains necessary for correctness on wide-vector hardware, and the expanded test ensures the bug cannot regress silently in future. --- .../opencv2/core/hal/intrin_rvv_scalable.hpp | 25 ++++++++++--------- modules/core/src/matmul.simd.hpp | 3 +-- modules/core/test/test_intrin_utils.hpp | 14 +++++------ 3 files changed, 20 insertions(+), 22 deletions(-) diff --git a/modules/core/include/opencv2/core/hal/intrin_rvv_scalable.hpp b/modules/core/include/opencv2/core/hal/intrin_rvv_scalable.hpp index 67c9d12741..ca82d140f8 100644 --- a/modules/core/include/opencv2/core/hal/intrin_rvv_scalable.hpp +++ b/modules/core/include/opencv2/core/hal/intrin_rvv_scalable.hpp @@ -2145,28 +2145,29 @@ inline v_float64 v_dotprod_expand_fast(const v_int32& a, const v_int32& b, const { return v_add(v_dotprod_expand_fast(a, b) , c); } #endif -// TODO: only 128 bit now. inline v_float32 v_matmul(const v_float32& v, const v_float32& mat0, const v_float32& mat1, const v_float32& mat2, const v_float32& mat3) { - vfloat32m2_t res; - res = __riscv_vfmul_vf_f32m2(mat0, v_extract_n(v, 0), VTraits::vlanes()); - res = __riscv_vfmacc_vf_f32m2(res, v_extract_n(v, 1), mat1, VTraits::vlanes()); - res = __riscv_vfmacc_vf_f32m2(res, v_extract_n(v, 2), mat2, VTraits::vlanes()); - res = __riscv_vfmacc_vf_f32m2(res, v_extract_n(v, 3), mat3, VTraits::vlanes()); - return res; + const int vl = VTraits::vlanes(); + vuint32m2_t idx = __riscv_vand(__riscv_vid_v_u32m2(vl), 0xfffffffc, vl); + v_float32 v0 = __riscv_vrgather(v, idx, vl); + v_float32 v1 = __riscv_vrgather(v, __riscv_vadd(idx, 1, vl), vl); + v_float32 v2 = __riscv_vrgather(v, __riscv_vadd(idx, 2, vl), vl); + v_float32 v3 = __riscv_vrgather(v, __riscv_vadd(idx, 3, vl), vl); + return v_fma(v0, mat0, v_fma(v1, mat1, v_fma(v2, mat2, v_mul(v3, mat3)))); } -// TODO: only 128 bit now. inline v_float32 v_matmuladd(const v_float32& v, const v_float32& mat0, const v_float32& mat1, const v_float32& mat2, const v_float32& a) { - vfloat32m2_t res = __riscv_vfmul_vf_f32m2(mat0, v_extract_n(v,0), VTraits::vlanes()); - res = __riscv_vfmacc_vf_f32m2(res, v_extract_n(v,1), mat1, VTraits::vlanes()); - res = __riscv_vfmacc_vf_f32m2(res, v_extract_n(v,2), mat2, VTraits::vlanes()); - return __riscv_vfadd(res, a, VTraits::vlanes()); + const int vl = VTraits::vlanes(); + vuint32m2_t idx = __riscv_vand(__riscv_vid_v_u32m2(vl), 0xfffffffc, vl); + v_float32 v0 = __riscv_vrgather(v, idx, vl); + v_float32 v1 = __riscv_vrgather(v, __riscv_vadd(idx, 1, vl), vl); + v_float32 v2 = __riscv_vrgather(v, __riscv_vadd(idx, 2, vl), vl); + return v_fma(v0, mat0, v_fma(v1, mat1, v_fma(v2, mat2, a))); } inline void v_cleanup() {} diff --git a/modules/core/src/matmul.simd.hpp b/modules/core/src/matmul.simd.hpp index f089c90a01..5645617dd1 100644 --- a/modules/core/src/matmul.simd.hpp +++ b/modules/core/src/matmul.simd.hpp @@ -1596,8 +1596,7 @@ static void transform_32f( const float* src, float* dst, const float* m, int len, int scn, int dcn ) { // Disabled for RISC-V Vector (scalable), because of: -// 1. v_matmuladd for RVV is 128-bit only but not scalable, this will fail the test `Core_Transform.accuracy`. -// 2. Both gcc and clang can autovectorize this, with better performance than using Universal intrinsic. +// 1. Both gcc and clang can autovectorize this, with better performance than using Universal intrinsic. #if (CV_SIMD || CV_SIMD_SCALABLE) && !defined(__aarch64__) && !defined(_M_ARM64) && !defined(_M_ARM64EC) && !(CV_TRY_RVV && CV_RVV) int x = 0; if( scn == 3 && dcn == 3 ) diff --git a/modules/core/test/test_intrin_utils.hpp b/modules/core/test/test_intrin_utils.hpp index 8c2c69c79f..2a0ce37611 100644 --- a/modules/core/test/test_intrin_utils.hpp +++ b/modules/core/test/test_intrin_utils.hpp @@ -1519,9 +1519,8 @@ template struct TheTest R v = dataV, a = dataA, b = dataB, c = dataC, d = dataD; Data res = v_matmul(v, a, b, c, d); - // for (int i = 0; i < VTraits::vlanes(); i += 4) - // { - int i = 0; + for (int i = 0; i < VTraits::vlanes(); i += 4) + { for (int j = i; j < i + 4; ++j) { SCOPED_TRACE(cv::format("i=%d j=%d", i, j)); @@ -1531,12 +1530,11 @@ template struct TheTest + dataV[i + 3] * dataD[j]; EXPECT_COMPARE_EQ(val, res[j]); } - // } + } Data resAdd = v_matmuladd(v, a, b, c, d); - // for (int i = 0; i < VTraits::vlanes(); i += 4) - // { - i = 0; + for (int i = 0; i < VTraits::vlanes(); i += 4) + { for (int j = i; j < i + 4; ++j) { SCOPED_TRACE(cv::format("i=%d j=%d", i, j)); @@ -1546,7 +1544,7 @@ template struct TheTest + dataD[j]; EXPECT_COMPARE_EQ(val, resAdd[j]); } - // } + } return *this; }