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

Universal intrinsics implementation with RISC-V vector extension #18228

Merged
merged 1 commit into from Dec 2, 2020

Conversation

joy2myself
Copy link
Contributor

@joy2myself joy2myself commented Aug 31, 2020

This PR From GSoC Project "Optimize OpenCV for RISC-V"

Project introduction

The main objective of the project is adding implementation of OpenCV wide universal intrinsics for RISC-V vector in OpenCV Hardware Acceleration Layer (HAL).
This PR is now working in progress.

Two previous PRs related to the project

Following two previous PRs mainly provided the toolchain files for cross-compiling OpenCV with target riscv64 on Linux platform. The first PR is with Clang and the second is with riscv-gnu-toolchain.
#17922
#18227

Main contents of my work

Added toolchain file for target riscv64-clang.
Added toolchain file for target riscv64-gcc.
Added universal intrinsics implementation with RISC-V vector extension.
Added some native intrinsics in C++ that are unsupported by compiler temporarily.

Current progress

Universal intrinsics implementation with RISC-V vector extension is almost done. But the way universal intrinsic vector types are declared right now needs some changes. See the following issue for specific reasons.
riscv-collab/riscv-gnu-toolchain#701
Use empty functions instead of missing native intrinsics temporarily.

Near future work

Change the universal intrinsic vector types with in-memory vectors and add extra loads/stores to get functional code.
Implement missing native intrinsics in C++ which are now still empty functions.

Long future work

Performance improvements. Maybe the current universal intrinsic framework need some change to fit scalable vector architecture.
RISC-V vector extension and rvv-intrinsics are still in the process of development. The compiler support is not completely ready at present and there may be changes in riscv-v-spec and rvv-intrinsics in the future. Therefore, this implementation needs to be maintained continuously according to the development of RISC-V.

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 other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to 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

@joy2myself joy2myself changed the title Universal intrinsics implementation with risc-v vector extension Universal intrinsics implementation with RISC-V vector extension Aug 31, 2020
@asmorkalov asmorkalov self-requested a review August 31, 2020 14:40
Copy link
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

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

/home/alexander/Projects/riscv/opencv/modules/core/test/test_intrin_utils.hpp:337:46: error: no matching function for call to 'v_reinterpret_as_f32(cv::hal_baseline::v_float64x2&)'
  337 |         v_float32 vf32 = v_reinterpret_as_f32(r1); out.a.clear(); v_store((float*)out.a.d, vf32); EXPECT_EQ(data.a, out.a);
      |                          ~~~~~~~~~~~~~~~~~~~~^~~~


v_uint8x16() {}
explicit v_uint8x16(vuint8m1_t v) : val(v) {}
v_uint8x16(uchar v0, uchar v1, uchar v2, uchar v3, uchar v4, uchar v5, uchar v6, uchar v7,
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy constructor and operator= required for build for all v_int, v_uint, v_float, etc structures.

@asmorkalov
Copy link
Contributor

/home/alexander/Projects/riscv/opencv/modules/core/test/test_intrin_utils.hpp:339:46: error: no matching function for call to 'v_reinterpret_as_f64(cv::hal_baseline::v_float32x4&)'
  339 |         v_float64 vf64 = v_reinterpret_as_f64(r1); out.a.clear(); v_store((double*)out.a.d, vf64); EXPECT_EQ(data.a, out.a);
      | 

@asmorkalov
Copy link
Contributor

In file included from /home/alexander/Projects/riscv/opencv/modules/core/test/test_intrin128.simd.hpp:19,
                 from /home/alexander/Projects/riscv/opencv/modules/core/test/test_intrin.cpp:6:
/home/alexander/Projects/riscv/opencv/modules/core/test/test_intrin_utils.hpp: In instantiation of 'opencv_test::hal::intrin128::cpu_baseline::TheTest<R>& opencv_test::hal::intrin128::cpu_baseline::TheTest<R>::test_popcount() [with R = cv::hal_baseline::v_int8x16]':
/home/alexander/Projects/riscv/opencv/modules/core/test/test_intrin_utils.hpp:1641:24:   required from here
/home/alexander/Projects/riscv/opencv/modules/core/test/test_intrin_utils.hpp:829:35: error: conversion from 'cv::hal_baseline::v_int8x16' to non-scalar type 'opencv_test::hal::intrin128::cpu_baseline::Data<cv::hal_baseline::v_uint8x16>' requested
  829 |         Data<Ru> resB = v_popcount(a);
      |                         ~~~~~~~~~~^~~

@vpisarev
Copy link
Contributor

vpisarev commented Dec 1, 2020

👍

@vpisarev
Copy link
Contributor

vpisarev commented Dec 1, 2020

@asmorkalov, you requested some changes. Are you happy with what has been done. I tested this PR and from side I do not see any showstoppers, except for the two:

@joy2myself:

  1. please, squash the commits into one; I've sent you the instructions by e-mail.
  2. please, copy our checklist about licenses and other conditions from another pull request into the first message of this pull request and click the checkboxes. Without your "written" agreement with our conditions that we put into each pull request we cannot integrate your patch

@joy2myself
Copy link
Contributor Author

Hi dear @asmorkalov and @vpisarev ,

I reverted some unnessesary commits before squash. I tested the current version and it works as well as before. Even so, I recommend that you test it again before merging to make sure there are no problems.

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

5 participants