Skip to content

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

Merged
asmorkalov merged 3 commits intoopencv:4.xfrom
hanliutong:rvv-64f
May 21, 2024
Merged

Fix v_round and enable unit tests for scalable universal intrinsic 64F type.#25586
asmorkalov merged 3 commits intoopencv:4.xfrom
hanliutong:rvv-64f

Conversation

@hanliutong
Copy link
Copy Markdown
Contributor

@hanliutong hanliutong commented May 15, 2024

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

@asmorkalov asmorkalov added this to the 4.10.0 milestone May 15, 2024
@asmorkalov asmorkalov requested a review from mshabunin May 15, 2024 06:25
@hanliutong hanliutong mentioned this pull request May 17, 2024
Closed
4 tasks
@hanliutong
Copy link
Copy Markdown
Contributor Author

hanliutong commented May 17, 2024

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
Copy Markdown
Contributor

mshabunin commented May 20, 2024 •
edited
Loading

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
Copy Markdown
Contributor Author

hanliutong commented May 21, 2024

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.

mshabunin
mshabunin approved these changes May 21, 2024
View reviewed changes
Copy link
Copy Markdown
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
Junyan721113 pushed a commit to plctlab/opencv that referenced this pull request May 22, 2024
@hanliutong @Junyan721113
Merge pull request opencv#25586 from hanliutong:rvv-64f
05497a7 
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
@hanliutong hanliutong deleted the rvv-64f branch June 4, 2024 08:08
@mshabunin mshabunin mentioned this pull request Jun 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mshabunin mshabunin mshabunin approved these changes

Assignees

@mshabunin mshabunin

Labels

optimization platform: riscv

Projects

None yet

Milestone

4.10.0

Development

Successfully merging this pull request may close these issues.

3 participants

@hanliutong @mshabunin @asmorkalov