GAPI Fluid: Dynamic dispatching for Split3 kernel by alexgiving · Pull Request #21441 · opencv/opencv

alexgiving · 2022-01-13T14:56:07Z

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 other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to 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

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

anna-khakimova · 2022-01-21T10:37:37Z

modules/gapi/src/backends/fluid/gfluidcore_func.dispatch.cpp


+#define SPLIT3_SIMD(SRC, DST)                                            \
+int split3_simd(const SRC in[], DST out1[], DST out2[],                  \
+                DST out3[], const int width)                             \


It makes no sense to make this function a macro for this kernel. Make this function as a normal function.

anna-khakimova · 2022-01-21T10:39:12Z

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

-    #endif
+#if CV_SIMD
+        w = split3_simd(in, out1, out2, out3, width);



Please remove empty line.

anna-khakimova · 2022-01-21T10:41:01Z

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

+//-------------------------
+
+#define SPLIT3_SIMD(SRC, DST)                                       \
+int split3_simd(const SRC in[], DST out1[], DST out2[],             \


It makes no sense to make this function as a macro for this kernel. Make this function as a normal function with uchar type for all parameters.

anna-khakimova · 2022-01-21T10:43:51Z

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

 #undef ABSDIFFC_SIMD

+#define SPLIT3_SIMD(DST1, DST2, DST3, SRC)                          \
+int split3_simd(const SRC in[], DST1 out1[], DST2 out2[],           \


And there is only one DST type. Three different DST types are redundant.
This construction must be forward declaration for this function. So this macro would be the same as this

NOTE:
It makes no sense to make this function as a macro for this kernel. Please make this function as a usual function.

anna-khakimova · 2022-01-21T10:53:59Z

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

+    int x = 0;                                                      \
+    for (; x <= width - nlanes; x += nlanes)                        \
+    {                                                               \
+        v_uint8 a, b, c;                                            \


It would be better to move creation and initialization of these variables from the loop.

anna-khakimova

LGTM 👍

alalek · 2022-01-24T08:30:05Z

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

+{
+    constexpr int nlanes = v_uint8::nlanes;
+    int x = 0;
+    v_uint8 a, b, c;


v_uint8 a, b, c;

Why is it outside of the loop body?

Keep variables declaration near their usage.

Is it necessary to create vectors at each iteration?

Is necessary to keep vector values between iterations?

Is necessary to keep vector values between iterations?

@alalek
There is no necessity to do setzero() for each iteration of the loop since they are initialized by the v_deinterleave() function before calculations. So creation and first initialization with zeroes can be moved from the loop.

Where do you see "setzero()" here?

Good rule of thumb for local variables to declare them in place where they are used.
No need to cross for loop boundary or any other, especially for SIMD register wrappers.

dmatveev

👍

alexgiving added optimization category: g-api / gapi labels Jan 13, 2022

alexgiving requested a review from anna-khakimova January 17, 2022 12:51

anna-khakimova reviewed Jan 21, 2022

View reviewed changes

alexgiving force-pushed the atrutnev/split3_simd_fluid branch from 2d19f3f to 0b8d606 Compare January 21, 2022 12:10

anna-khakimova approved these changes Jan 24, 2022

View reviewed changes

anna-khakimova assigned dmatveev Jan 24, 2022

alalek reviewed Jan 24, 2022

View reviewed changes

dmatveev added this to the 4.6.0 milestone Jan 24, 2022

dmatveev approved these changes Jan 24, 2022

View reviewed changes

alexgiving force-pushed the atrutnev/split3_simd_fluid branch 2 times, most recently from 8bb494b to 5e89b9a Compare January 25, 2022 12:03

anna-khakimova requested a review from alalek January 25, 2022 13:44

opencv-pushbot merged commit 9238316 into opencv:4.x Jan 25, 2022

Dynamic Dispatching for split3

5e89b9a

alalek mentioned this pull request Feb 22, 2022

(5.x) Merge 4.x #21651

Merged

Uh oh!

Conversation

alexgiving commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

anna-khakimova Jan 21, 2022 • 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!

anna-khakimova Jan 21, 2022 • 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!

anna-khakimova Jan 21, 2022 • 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!

anna-khakimova left a comment

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 Jan 25, 2022 • 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!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alexgiving commented Jan 13, 2022 •

edited

Loading

anna-khakimova Jan 21, 2022 •

edited

Loading

anna-khakimova Jan 21, 2022 •

edited

Loading

anna-khakimova Jan 21, 2022 •

edited

Loading

anna-khakimova Jan 25, 2022 •

edited

Loading