GAPI Fluid: Enable dynamic dispatching for AbsDiffC kernel. #21204

anna-khakimova · 2021-12-06T11:05:57Z

Moved SIMD for GAPI Fluid AbsDiffC kernel to enable dynamic dispatching.

Performance report:

FluidAbsDiffCSIMDEnableDynamicDispatch.xlsx

force_builders=Linux AVX2,Custom,Custom Win,Custom Mac
build_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
Xbuild_gapi_standalone:Mac=ade-0.1.1f
build_gapi_standalone:Linux x64 Debug=ade-0.1.1f

build_image:Custom=centos:7
buildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

Xbuild_image:Custom=ubuntu-openvino-2021.3.0:20.04
build_image:Custom Win=openvino-2021.4.1
build_image:Custom Mac=openvino-2021.2.0

buildworker:Custom Win=windows-3

test_modules:Custom=gapi,python2,python3,java
test_modules:Custom Win=gapi,python2,python3,java
test_modules:Custom Mac=gapi,python2,python3,java

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
Xtest_opencl:Custom=OFF
Xtest_bigdata:Custom=1
Xtest_filter:Custom=*

CPU_BASELINE:Custom Win=AVX512_SKX
CPU_BASELINE:Custom=SSE4_2

sivanov-work · 2021-12-07T05:31:52Z

modules/gapi/src/backends/fluid/gfluidcore.cpp


    int w = 0;
 #if CV_SIMD
-    w = absdiffc_simd(in, scalar, out, width, chan);
+    w = absdiffc_simd(in, scalar, out, length, chan);


it was a bug before?

No, it wasn't a bug. I just took the length calculation to a higher level.

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

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).

sivanov-work · 2021-12-07T05:32:51Z

modules/gapi/src/backends/fluid/gfluidcore.cpp

@@ -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;


just for interest: why all types is signed? is it by historical reasons?

Yes, it is for history :-)

sivanov-work · 2021-12-07T05:37:14Z

modules/gapi/src/backends/fluid/gfluidcore_func.hpp

@@ -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[],      \


what are differences with function in dispatch https://github.com/opencv/opencv/pull/21204/files#diff-693224a96b2408435e643bd3353f4a66954b6d57accd21fac061d14677d5c656R169 ?

There are no difference. This structure was organized for dynamic dispatching support.

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

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

gfluidcore_func.hpp is header file for forward declaration of wrappers in the gfluidcore_func.dispatch.cpp. It's necessary for C++ compiler.

i suggest to rename MACRO as ABSDIFFC_SIMD_DECLARATION & ABSDIFFC_SIMD_DEFINITION or 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.

sivanov-work · 2021-12-07T05:39:16Z

modules/gapi/src/backends/fluid/gfluidcore_func.simd.hpp

+//-------------------------
+
+#define ABSDIFFC_SIMD(SRC)                                                          \
+int absdiffc_simd(const SRC in[], const float scalar[], SRC out[],                  \


always_inline ?

Because of dynamic dispatching support, any key words before return type break compilation.

any key words before return type break compilation.

could you please provide observed error message?

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.

sivanov-work

LGTM

dmatveev

A couple of questions

dmatveev · 2021-12-10T07:59:54Z

modules/gapi/perf/common/gapi_core_perf_tests.hpp

@@ -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>> {};


Shouldn't this be exact? why do we need a comparison function here?

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.

No, the question was why this check is not exact any more but we discussed this double/float change on call. Thanks

dmatveev · 2021-12-10T08:00:06Z

modules/gapi/perf/common/gapi_core_perf_tests_inl.hpp

+    compare_f cmpF = get<0>(GetParam());
+    cv::Size sz_in = get<1>(GetParam());
+    MatType type = get<2>(GetParam());


dmatveev

👍

anna-khakimova added optimization category: g-api / gapi labels Dec 6, 2021

anna-khakimova added this to the 4.5.5 milestone Dec 6, 2021

anna-khakimova requested a review from sivanov-work December 6, 2021 11:05

GAPI Fluid: Enable dynamic dispatching for AbsDiffC kernel.

a4d6bcb

anna-khakimova force-pushed the ak/move_simd_absdiffc branch from 1a528b6 to a4d6bcb Compare December 6, 2021 12:16

sivanov-work reviewed Dec 7, 2021

View reviewed changes

anna-khakimova requested review from sivanov-work and alalek December 8, 2021 10:22

dmatveev self-assigned this Dec 8, 2021

sivanov-work approved these changes Dec 9, 2021

View reviewed changes

dmatveev reviewed Dec 10, 2021

View reviewed changes

dmatveev approved these changes Dec 13, 2021

View reviewed changes

opencv-pushbot merged commit f1053d4 into opencv:4.x Dec 13, 2021

alalek mentioned this pull request Dec 30, 2021

(5.x) Merge 4.x #21371

Merged

alalek mentioned this pull request Feb 22, 2022

(5.x) Merge 4.x #21651

Merged

Uh oh!

GAPI Fluid: Enable dynamic dispatching for AbsDiffC kernel. #21204

GAPI Fluid: Enable dynamic dispatching for AbsDiffC kernel. #21204

Uh oh!

Conversation

anna-khakimova commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anna-khakimova Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anna-khakimova Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivanov-work left a comment

Choose a reason for hiding this comment

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anna-khakimova Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anna-khakimova commented Dec 6, 2021 •

edited

Loading

anna-khakimova Dec 8, 2021 •

edited

Loading

anna-khakimova Dec 8, 2021 •

edited

Loading

anna-khakimova Dec 10, 2021 •

edited

Loading