Skip to content

[hal][neon] Optimize the v_dotprod_fast intrinsics for aarch64.#19486

Merged
alalek merged 5 commits intoopencv:3.4from
fpetrogalli:dotprod_fast-3.4
Feb 11, 2021
Merged

[hal][neon] Optimize the v_dotprod_fast intrinsics for aarch64.#19486
alalek merged 5 commits intoopencv:3.4from
fpetrogalli:dotprod_fast-3.4

Conversation

@fpetrogalli
Copy link
Copy Markdown
Contributor

@fpetrogalli fpetrogalli commented Feb 9, 2021 •
edited by alalek
Loading

On Armv8 in AArch64 execution mode, we can skip the sequence

   v<op>_<ty>(vget_high_<ty>(x), vget_high_<ty>(y))

in favour of

   v<op>_high_<ty>(x, y)

This has better changes for recent compilers to use less data movement
operations and better register allocation. See for example:

https://godbolt.org/z/bPq7vd

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 other license that is incompatible with OpenCV
  • [ X ] 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
force_builders=linux,docs,ARMv8,ARMv7

[hal][neon] Optimize the v_dotprod_fast intrinsics for aarch64.
f9caaf3 
On Armv8 in AArch64 execution mode, we can skip the sequence

   v<op>_<ty>(vget_high_<ty>(x), vget_high_<ty>(y))

in favour of

   v<op>_high_<ty>(x, y)

This has better changes for recent compilers to use less data movement
operations and better register allocation. See for example:

   https://godbolt.org/z/bPq7vd
@fpetrogalli
Copy link
Copy Markdown
Contributor Author

fpetrogalli commented Feb 9, 2021

Here is the speedup (as in new version / old version) I measured on graviton2 on AWS (neoverse N1).

I used perf to measure the following command:

./bin/opencv_perf_core --gtest_filter=*PerfHamming*'

The total cycle count of the function cv::cpu_baseline::dotProd_32s went down to 0.86x in the version with the patch, compared to the baseline in master.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Feb 9, 2021

@fpetrogalli Please take a look on CI builds for iOS. The patch breaks the build.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Feb 9, 2021

The same for Android on arm-v7a.

@fpetrogalli
Copy link
Copy Markdown
Contributor Author

fpetrogalli commented Feb 9, 2021

@asmorkalov - yes! Thank you, I have noticed. I am trying to reproduce the error. I am quite surprised because the macro CV_SIMD128_64F is conditionally defined when the __aarch64__ is, and I have no clue (yet) why this is happening on armv7. I am probably missing something trivial.

@alalek
Copy link
Copy Markdown
Member

alalek commented Feb 10, 2021

/cc @tomoaki0705

alalek
alalek reviewed Feb 10, 2021
View reviewed changes
tomoaki0705
tomoaki0705 suggested changes Feb 10, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@tomoaki0705 tomoaki0705 left a comment

Choose a reason for hiding this comment

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

Generally looks good.
Please see small notes from me.

alalek
alalek reviewed Feb 10, 2021
View reviewed changes
[hal][neon] Fix macro definition for AArch64.
720da8a 
The fix is needed to prevent warnings when building for Armv7.
alalek
alalek approved these changes Feb 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

@fpetrogalli Thank you for contribution!
@tomoaki0705 Thank you for review!

@alalek alalek merged commit 6ee23c9 into opencv:3.4 Feb 11, 2021
@fpetrogalli fpetrogalli deleted the dotprod_fast-3.4 branch February 11, 2021 13:57
@alalek alalek mentioned this pull request Feb 12, 2021
Merged
@jondea jondea mentioned this pull request Mar 24, 2021
Merged
6 tasks
@alalek alalek mentioned this pull request Apr 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek approved these changes

@terfendail terfendail Awaiting requested review from terfendail

+1 more reviewer

@tomoaki0705 tomoaki0705 tomoaki0705 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alalek alalek

Labels

optimization platform: arm ARM boards related issues: RPi, NVIDIA TK/TX, etc

Projects

None yet

Milestone

3.4.14

Development

Successfully merging this pull request may close these issues.

4 participants

@fpetrogalli @asmorkalov @alalek @tomoaki0705