Skip to content

Avoid extra memset#25184

Merged
asmorkalov merged 1 commit intoopencv:4.xfrom
dkurt:avoid_extra_memset
Mar 25, 2024
Merged

Avoid extra memset#25184
asmorkalov merged 1 commit intoopencv:4.xfrom
dkurt:avoid_extra_memset

Conversation

@dkurt
Copy link
Copy Markdown
Member

@dkurt dkurt commented Mar 8, 2024

Pull Request Readiness Checklist

Just an experiment to avoid extra usage of memset.

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

@opencv-alalek opencv-alalek requested a review from vpisarev March 15, 2024 02:24
@dkurt dkurt marked this pull request as ready for review March 15, 2024 14:11
@asmorkalov asmorkalov requested a review from fengyuentau March 23, 2024 11:18
@asmorkalov asmorkalov added this to the 4.10.0 milestone Mar 23, 2024
@fengyuentau
Copy link
Copy Markdown
Member

fengyuentau commented Mar 25, 2024

Do we have tests for fp16 convolution? cc @zihaomu

@zihaomu
Copy link
Copy Markdown
Member

zihaomu commented Mar 25, 2024

Yes, we have the FP16 accuracy test, which has the flag of DNN_TARGET_CPU_FP16.

opencv/modules/dnn/test/test_onnx_importer.cpp

Line 2352 in 52f3f5a

if (target == DNN_TARGET_OPENCL_FP16 || target == DNN_TARGET_MYRIAD || target == DNN_TARGET_CPU_FP16)

@fengyuentau
Copy link
Copy Markdown
Member

fengyuentau commented Mar 25, 2024

Yes, we have the FP16 accuracy test, which has the flag of DNN_TARGET_CPU_FP16.

opencv/modules/dnn/test/test_onnx_importer.cpp

Line 2352 in 52f3f5a

if (target == DNN_TARGET_OPENCL_FP16 || target == DNN_TARGET_MYRIAD || target == DNN_TARGET_CPU_FP16)

It is turned off by default: https://github.com/opencv/opencv/actions/runs/8200307720/job/22436426876?pr=25184#step:17:12821. Tests for a single convolution layer on fp16?

@zihaomu
Copy link
Copy Markdown
Member

zihaomu commented Mar 25, 2024 •
edited
Loading

https://github.com/opencv/opencv/actions/runs/8200307720/job/22436426876?pr=25184#step:17:12955, the default is ON, and for some specific cases we turned it off since the accuracy issue was introduced by FP16.

@dkurt
Copy link
Copy Markdown
Member Author

dkurt commented Mar 25, 2024

@fengyuentau, @zihaomu, does it make sense to verify that this PR will pass on 5.x branch too?

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Mar 25, 2024

@dkurt You may create yet another PR to 5.x with the same change. I'll merge it independently.

@dkurt dkurt mentioned this pull request Mar 25, 2024
Closed
6 tasks
@asmorkalov asmorkalov merged commit fc34554 into opencv:4.x Mar 25, 2024
@asmorkalov asmorkalov mentioned this pull request Apr 1, 2024
Merged
@dkurt dkurt deleted the avoid_extra_memset branch November 20, 2025 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zihaomu zihaomu zihaomu approved these changes

@fengyuentau fengyuentau fengyuentau approved these changes

@vpisarev vpisarev Awaiting requested review from vpisarev

Assignees

No one assigned

Labels

category: dnn optimization

Projects

None yet

Milestone

4.10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@dkurt @fengyuentau @zihaomu @asmorkalov