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

Fix v_round and enable unit tests for scalable universal intrinsic 64F type. #25586

Merged
merged 3 commits into from
May 21, 2024

Conversation

hanliutong
Copy link
Contributor

This may be a legacy issue from the previous PR #24325. I don't quite remember why the float 64 part of the unit test was not enabled at that time.

Whatever, this patch enables the unit tests for scalable 64F type , and makes the necessary modifications to the RVV backend to make the tests pass.

This patch is compiled by GCC 14 and LLVM 17 &18, and tested on QEMU and k230.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • 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
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@hanliutong
Copy link
Contributor Author

About #24746:

  • The root cause of the problem was an incorrect implementation of v_round in RVV backend, which has now been fixed.
-- return vfncvt_x(vset(vlmul_ext_f64m2(vfadd(a, 1e-6, VTraits<v_float64>::vlanes())), 1, b), VTraits<v_float32>::vlanes());
++ return vfncvt_x(vset(vlmul_ext_f64m2(a), 1, b), VTraits<v_float32>::vlanes());
  • The indirect cause is that our unit test for Universal Intrinsic was
  1. not enabled for scalable backends on 64F type previously, fixed by:
-- #if CV_SIMD_64F
++ #if (CV_SIMD_64F || CV_SIMD_SCALABLE_64F)
  1. not covered testing of v_round(v_f64, v_f64), fixed by
++ TheTest & test_round_pair_f64()
++ {
++ ...
++ }

@hanliutong hanliutong changed the title Enable unit tests for scalable universal intrinsic 64F type. Fix v_round and enable unit tests for scalable universal intrinsic 64F type. May 17, 2024
@mshabunin
Copy link
Contributor

mshabunin commented May 20, 2024

It seems LLVM 17 doesn't support vcreate intrinsic, I've got an error:

/opencv/modules/core/include/opencv2/core/hal/intrin_rvv_scalable.hpp:1540:25: error: use of undeclared identifier '__riscv_vcreate_v_f64m1_f64m2'
 1540 |     vfloat64m2_t temp = __riscv_vcreate_v_f64m1_f64m2(a0, a1);
      |                         ^

Toolchain: LLVM 17.0.6
CMake options (excerpt):

    -DRISCV_CLANG_BUILD_ROOT=${llvm_root} \
    -DRISCV_GCC_INSTALL_ROOT=${gcc_path} \
    -DCMAKE_TOOLCHAIN_FILE=/opencv/platforms/linux/riscv64-clang.toolchain.cmake \
    -DBUILD_SHARED_LIBS=OFF \
    -DWITH_OPENCL=OFF \
    -DRISCV_RVV_SCALABLE=ON \
    -DCPU_BASELINE=RVV \
    -DCPU_BASELINE_REQUIRE=RVV \
    -DCMAKE_BUILD_TYPE=Release

My attemt to recreate: https://godbolt.org/z/f9oh77dqM (clang 18 works). Perhaps intrinsics version check should be >0.12? - it doesn't seem to help.

UPD: yes, it has been added in clang 18 - llvm/llvm-project#70355. Since intrinsics version has not been bumped we need to check clang version if we want to use this intrinsic.

@hanliutong
Copy link
Contributor Author

Bug reproduce. Seem like the "if code" and the "then code" generate the same code, so maybe we can just not use vctreat and leave it there (if 0) until the intrinsic frozen and compiler fully supports the intrinsic spec.

Copy link
Contributor

@mshabunin mshabunin left a comment

Choose a reason for hiding this comment

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

Thank you!

@asmorkalov asmorkalov merged commit e525401 into opencv:4.x May 21, 2024
29 of 30 checks passed
Junyan721113 pushed a commit to plctlab/opencv that referenced this pull request May 22, 2024
Fix v_round and enable unit tests for scalable universal intrinsic 64F type. opencv#25586

This may be a legacy issue from the previous PR opencv#24325. I don't quite remember why the float 64 part of the unit test was not enabled at that time.

Whatever, this patch enables the unit tests for scalable 64F type , and makes the necessary modifications to the RVV backend to make the tests pass.

This patch is compiled by GCC 14 and LLVM 17 &18, and tested on QEMU and k230.

### Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [ ] I agree to contribute to the project under Apache 2 License.
- [ ] 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
- [ ] The PR is proposed to the proper branch
- [ ] There is a reference to the original bug report and related work
- [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [ ] The feature is well documented and sample code can be built with the project CMake
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.

None yet

3 participants