Skip to content

core: fix Core_GEMM.accuracy failure on recent macOS#25454

Merged
asmorkalov merged 1 commit intoopencv:4.xfrom
fengyuentau:fix_core_gemm_acc
Apr 19, 2024
Merged

core: fix Core_GEMM.accuracy failure on recent macOS#25454
asmorkalov merged 1 commit intoopencv:4.xfrom
fengyuentau:fix_core_gemm_acc

Conversation

@fengyuentau
Copy link
Copy Markdown
Member

@fengyuentau fengyuentau commented Apr 19, 2024 •
edited
Loading

Resolves #25302

Reproducer: https://github.com/opencv/ci-gha-workflow/actions/runs/8747714722/job/24006610667?pr=171#step:12:1041

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

@fengyuentau fengyuentau self-assigned this Apr 19, 2024
@fengyuentau fengyuentau added this to the 4.10.0 milestone Apr 19, 2024
@asmorkalov asmorkalov added the bug label Apr 19, 2024
@asmorkalov asmorkalov requested a review from vpisarev April 19, 2024 06:28
@fengyuentau fengyuentau changed the title remove manual unrolling that causes problem core: fix Core_GEMM.accuracy failure on recent macOS Apr 19, 2024
@asmorkalov asmorkalov assigned vpisarev and unassigned fengyuentau Apr 19, 2024
@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented Apr 19, 2024

It is even weirder that if I set unrolling by factor of 2 instead of 4, it passes. Could it be something related to the compiler?

                 #if CV_ENABLE_UNROLLED
                for(; j <= m - 2; j += 2 )
                {
                    WT t0 = d_buf[j] + WT(b_data[j])*al;
                    WT t1 = d_buf[j+1] + WT(b_data[j+1])*al;
                    d_buf[j] = t0;
                    d_buf[j+1] = t1;
                    // t0 = d_buf[j+2] + WT(b_data[j+2])*al;
                    // t1 = d_buf[j+3] + WT(b_data[j+3])*al;
                    // d_buf[j+2] = t0;
                    // d_buf[j+3] = t1;
                }
                #endif

vpisarev
vpisarev approved these changes Apr 19, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@vpisarev vpisarev left a comment

Choose a reason for hiding this comment

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

yes, we discussed it. probably, it's some weird bug in latest Clang compiler. Given that modern compiler do great job with code vectorization, the version without manually unrolled loop is not only simpler, but probably faster as well

@opencv-alalek
Copy link
Copy Markdown
Contributor

opencv-alalek commented Apr 19, 2024

Are there some aliased pointers on the same memory for reading and writing? (inplace processing)

@asmorkalov asmorkalov merged commit 5da17a4 into opencv:4.x Apr 19, 2024
@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented Apr 19, 2024 •
edited
Loading

Are there some aliased pointers on the same memory for reading and writing? (inplace processing)

Probably no.

  • d_buf is an AutoBuffer allocated in the domain of outside of the loop. It is set to zero for each element just before entering the problematic loop.
  • The source of b_data is a bit complecated. It is a parameter for the function and for test case 14, it is a piece of buffer copied from the original source B mat.

opencv/modules/core/src/matmul.simd.hpp

Lines 1246 to 1257 in 5da17a4

if( dj < d_size.width )
{
Size b_size;
if( !is_b_t )
b_size.width = dj, b_size.height = dk;
else
b_size.width = dk, b_size.height = dj;
_b_step = b_size.width*elem_size;
GEMM_CopyBlock( _b, b_step, b_buf, _b_step, b_size, elem_size );
_b = b_buf;
}

@fengyuentau fengyuentau deleted the fix_core_gemm_acc branch April 19, 2024 08:38
@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented Apr 19, 2024

Test is now green with this patch merged https://github.com/opencv/ci-gha-workflow/actions/runs/8750864630/job/24015266117?pr=171

@asmorkalov asmorkalov mentioned this pull request Apr 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vpisarev vpisarev vpisarev approved these changes

Assignees

@vpisarev vpisarev

Labels

bug category: core

Projects

None yet

Milestone

4.10.0

Development

Successfully merging this pull request may close these issues.

[macOS] test case Core_GEMM.accuracy fails on recent macOS

4 participants

@fengyuentau @opencv-alalek @vpisarev @asmorkalov