mirror of
https://github.com/opencv/opencv.git
synced 2026-07-28 06:43:01 +04:00
kevinylin88
01b23a0de5
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.