| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
GAPI Fluid: Enable dynamic dispatching for AbsDiffC kernel. #21204
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: Enable dynamic dispatching for AbsDiffC kernel. #21204
Conversation
anna-khakimova
force-pushed
the
ak/move_simd_absdiffc
branch
from
1a528b6 to
a4d6bcb
Compare
|
|
||
| int w = 0; | ||
| #if CV_SIMD | ||
| w = absdiffc_simd(in, scalar, out, width, chan); | ||
| w = absdiffc_simd(in, scalar, out, length, chan); |
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.
it was a bug before?
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.
No, it wasn't a bug. I just took the length calculation to a higher level.
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.
hm, it was width then became width * chan here - so data amount as increased in chan times. Looks like we didn't use by whole data range before (was bug) or processing more data (possible out-of-bound ) for now
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.
No, it wasn't so. Earlier we calculate length inside absdiffc_simd_c3() and absdiffc_simd_c1c2c4() functions and then used length for a loop. You can see it here for example. Now length is calculated 2 level up of call stack (i.e. here).
| @@ -1240,13 +1002,14 @@ static void run_absdiffc(Buffer &dst, const View &src, const float scalar[]) | |||
|
|
|||
| int width = dst.length(); | |||
| int chan = dst.meta().chan; | |||
| const int length = width * chan; | |||
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.
just for interest: why all types is signed? is it by historical reasons?
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 for history :-)
| @@ -129,6 +129,17 @@ MULC_SIMD(float, float) | |||
|
|
|||
| #undef MULC_SIMD | |||
|
|
|||
| #define ABSDIFFC_SIMD(T) \ | |||
| int absdiffc_simd(const T in[], const float scalar[], T 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.
what are differences with function in dispatch https://github.com/opencv/opencv/pull/21204/files#diff-693224a96b2408435e643bd3353f4a66954b6d57accd21fac061d14677d5c656R169 ?
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.
There are no difference. This structure was organized for dynamic dispatching support.
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.
Could you describe relationship by short schema between them please?
Because i see two functions with the similar names, with the similar agruments and in the same namespace .
May be one of fucntions should have *_impl suffic, may be i wrong
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.
Am i right in my assumption (it's definitely clear for me by now) that here we have function declaration
but in file before we have function definition?
If yes, then i suggest to rename MACRO as ABSDIFFC_SIMD_DECLARATION & ABSDIFFC_SIMD_DEFINITION or something like that
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.
gfluidcore_func.hpp is header file for forward declaration of wrappers in the gfluidcore_func.dispatch.cpp. It's necessary for C++ compiler.
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.
i suggest to rename MACRO as
ABSDIFFC_SIMD_DECLARATION&ABSDIFFC_SIMD_DEFINITIONor something like that
Ok. I would prefer to do this in the next PR together with MACRO for SIMD of the other kernels. All together.
| //------------------------- | ||
|
|
||
| #define ABSDIFFC_SIMD(SRC) \ | ||
| int absdiffc_simd(const SRC in[], const float scalar[], SRC 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.
always_inline ?
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.
Because of dynamic dispatching support, any key words before return type break compilation.
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.
any key words before return type break compilation.
could you please provide observed error message?
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.
It is implementation of external interface declared on the line 154 and used in other compilation modules, e.g. .dispatch.cpp. So it can't be inlined.
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
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.
A couple of questions
| @@ -50,7 +50,7 @@ namespace opencv_test | |||
| class MinPerfTest : public TestPerfParams<tuple<cv::Size, MatType, cv::GCompileArgs>> {}; | |||
| class MaxPerfTest : public TestPerfParams<tuple<cv::Size, MatType, cv::GCompileArgs>> {}; | |||
| class AbsDiffPerfTest : public TestPerfParams<tuple<cv::Size, MatType, cv::GCompileArgs>> {}; | |||
| class AbsDiffCPerfTest : public TestPerfParams<tuple<cv::Size, MatType, cv::GCompileArgs>> {}; | |||
| class AbsDiffCPerfTest : public TestPerfParams<tuple<compare_f, cv::Size, MatType, cv::GCompileArgs>> {}; | |||
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.
Shouldn't this be exact? why do we need a comparison function 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.
I don't quite understand the meaning of the question. Why does Alexey Trutnev add comparison functions to all GAPI performance tests in the PR? Probably to understand whether the kernels works correctly in the future.
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.
No, the question was why this check is not exact any more but we discussed this double/float change on call. Thanks
| compare_f cmpF = get<0>(GetParam()); | ||
| cv::Size sz_in = get<1>(GetParam()); | ||
| MatType type = get<2>(GetParam()); |
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.
tie()
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.
👍
Moved SIMD for GAPI Fluid AbsDiffC kernel to enable dynamic dispatching.
Performance report:
FluidAbsDiffCSIMDEnableDynamicDispatch.xlsx