potok/opencv
1
0
Fork 0
mirror of https://github.com/opencv/opencv.git synced 2026-07-29 23:33:05 +04:00
Code Packages Releases Wiki Activity

Merge pull request #29080 from kevinylin88:project4_kevinlin

Browse Source 
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

<!-- Note!!! If you are an automated agent, we have a special process for you: add 🤖🤖🤖 to the end of the PR title. -->

## 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<R>::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<R>::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.
This commit is contained in:
kevinylin88
2026-05-21 19:29:55 +08:00
committed by GitHub
parent f28a52398e
commit 01b23a0de5
3 changed files with 20 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
modules/core/include/opencv2/core/hal/intrin_rvv_scalable.hpp
+13 -12
View File
@@ -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<v_float32>::vlanes());
res = __riscv_vfmacc_vf_f32m2(res, v_extract_n(v, 1), mat1, VTraits<v_float32>::vlanes());
res = __riscv_vfmacc_vf_f32m2(res, v_extract_n(v, 2), mat2, VTraits<v_float32>::vlanes());
res = __riscv_vfmacc_vf_f32m2(res, v_extract_n(v, 3), mat3, VTraits<v_float32>::vlanes());
return res;
const int vl = VTraits<v_float32>::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<v_float32>::vlanes());
res = __riscv_vfmacc_vf_f32m2(res, v_extract_n(v,1), mat1, VTraits<v_float32>::vlanes());
res = __riscv_vfmacc_vf_f32m2(res, v_extract_n(v,2), mat2, VTraits<v_float32>::vlanes());
return __riscv_vfadd(res, a, VTraits<v_float32>::vlanes());
const int vl = VTraits<v_float32>::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() {}
modules/core/src/matmul.simd.hpp
+1 -2
View File
@@ -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 )
modules/core/test/test_intrin_utils.hpp
+6 -8
View File
@@ -1519,9 +1519,8 @@ template<typename R> struct TheTest
R v = dataV, a = dataA, b = dataB, c = dataC, d = dataD;
Data<R> res = v_matmul(v, a, b, c, d);
// for (int i = 0; i < VTraits<R>::vlanes(); i += 4)
// {
int i = 0;
for (int i = 0; i < VTraits<R>::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<typename R> struct TheTest
+ dataV[i + 3] * dataD[j];
EXPECT_COMPARE_EQ(val, res[j]);
}
// }
}
Data<R> resAdd = v_matmuladd(v, a, b, c, d);
// for (int i = 0; i < VTraits<R>::vlanes(); i += 4)
// {
i = 0;
for (int i = 0; i < VTraits<R>::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<typename R> struct TheTest
+ dataD[j];
EXPECT_COMPARE_EQ(val, resAdd[j]);
}
// }
}
return *this;
}