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
GAPI Fluid SIMD:Add support of new several types for the Merge3. #21797
GAPI Fluid SIMD:Add support of new several types for the Merge3. #21797
Conversation
anna-khakimova
commented
Mar 30, 2022
•
edited by asmorkalov
edited by asmorkalov
- Support of the new several types was added.
- Fixes for the Split/Merge and ConvertTo issues.
@@ -86,6 +86,25 @@ using cv::gapi::own::rintd; | |||
return; \ | |||
} | |||
|
|||
#define MERGE3_(T, OP, ...) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot find place where is it used, could you point me out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It is used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed offline:
to move out repeated conditions into beginning of function to simplify condition calculation complexity in actual type dispatching routing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -86,6 +86,25 @@ using cv::gapi::own::rintd; | |||
return; \ | |||
} | |||
|
|||
#define MERGE3_(T, OP, ...) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
@@ -86,6 +86,25 @@ using cv::gapi::own::rintd; | |||
return; \ | |||
} | |||
|
|||
#define MERGE3_(T, OP, ...) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed offline:
to move out repeated conditions into beginning of function to simplify condition calculation complexity in actual type dispatching routing
@@ -252,6 +252,7 @@ INSTANTIATE_TEST_CASE_P(Split4PerfTestCPU, Split4PerfTest, | |||
INSTANTIATE_TEST_CASE_P(Merge3PerfTestCPU, Merge3PerfTest, | |||
Combine(Values(AbsExact().to_compare_f()), | |||
Values(szSmall128, szVGA, sz720p, sz1080p), | |||
Values(CV_8U), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should other types be listed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
..and in GPU tests, too.
\ | ||
OP<T>(__VA_ARGS__); \ | ||
return; \ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Macro function bodies considered harmful, is there any reasonable issues with rewriting it to templates?
GAPI_DbgAssert(dst.length() == src1.length()); \ | ||
GAPI_DbgAssert(dst.length() == src2.length()); \ | ||
GAPI_DbgAssert(dst.length() == src3.length()); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the reason to verify this?
normally in the framework we rely on our output buffers which are allocated per our needs.
it is not coming somewhere from the outside.
@anna-khakimova @dmatveev Do you plan to finish the PR in meantime? |
@dmatveev @TolyaTalamanov Friendly reminder. |
1 similar comment
@dmatveev @TolyaTalamanov Friendly reminder. |
Will have a look at this one too. |
@rgarnov can you please have a look? |
753c404
to
42653f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, these changes don't bring any regressions on my end but so far extend Fluid's merge3 with extra data types.
Can be merged IMO
Geometric mean (ms)
Name of Test 4x anna anna
simd simd
vs
4x
(x-factor)
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 128x128, 8UC1, { gapi.kernel_package }) - 0.016 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 128x128, 16UC1, { gapi.kernel_package }) - 0.019 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 128x128, 16SC1, { gapi.kernel_package }) - 0.019 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 128x128, 32FC1, { gapi.kernel_package }) - 0.025 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 128x128, { gapi.kernel_package }) 0.016 - -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 640x480, 8UC1, { gapi.kernel_package }) - 0.099 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 640x480, 16UC1, { gapi.kernel_package }) - 0.175 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 640x480, 16SC1, { gapi.kernel_package }) - 0.178 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 640x480, 32FC1, { gapi.kernel_package }) - 0.347 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 640x480, { gapi.kernel_package }) 0.103 - -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 1280x720, 8UC1, { gapi.kernel_package }) - 0.255 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 1280x720, 16UC1, { gapi.kernel_package }) - 0.621 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 1280x720, 16SC1, { gapi.kernel_package }) - 0.613 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 1280x720, 32FC1, { gapi.kernel_package }) - 1.729 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 1280x720, { gapi.kernel_package }) 0.258 - -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 1920x1080, 8UC1, { gapi.kernel_package }) - 0.781 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 1920x1080, 16UC1, { gapi.kernel_package }) - 1.985 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 1920x1080, 16SC1, { gapi.kernel_package }) - 1.975 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 1920x1080, 32FC1, { gapi.kernel_package }) - 4.211 -
TestPerformance::Merge3PerfTestFluid/Merge3PerfTest::(compare_f, 1920x1080, { gapi.kernel_package }) 0.744 - -
TestPerformance::Merge4PerfTestFluid/Merge4PerfTest::(compare_f, 128x128, { gapi.kernel_package }) 0.019 0.020 0.96
TestPerformance::Merge4PerfTestFluid/Merge4PerfTest::(compare_f, 640x480, { gapi.kernel_package }) 0.127 0.129 0.98
TestPerformance::Merge4PerfTestFluid/Merge4PerfTest::(compare_f, 1280x720, { gapi.kernel_package }) 0.361 0.348 1.04
TestPerformance::Merge4PerfTestFluid/Merge4PerfTest::(compare_f, 1920x1080, { gapi.kernel_package }) 1.228 1.221 1.01
@opencv-alalek Could you take a look on CI failures. It looks like some update is required. |
Yep the main build runs ok:
But there's a separate ADE step where another (older) version is mentioned:
|
Because it is mentioned in CI configuration section of PR's description. |
Cool! Didn't know it still works given that there's Github Actions :) |
…supported_types GAPI Fluid SIMD:Add support of new several types for the Merge3 - Support of the new several types was added. - Fixes for the Split/Merge and ConvertTo issues.