DNN: Support some reduce layers of ONNX on CPU backend by zihaomu · Pull Request #21601 · opencv/opencv

zihaomu · 2022-02-12T13:03:56Z

The following Reduce layers of ONNX are supported:

ReduceL1
ReduceL2
ReduceLogSum
ReduceLogSumExp
ReduceMin
ReduceProd
ReduceSumSquare

The ReduceSum takes the axes as an input, instead of an attribute, more details, so it is not supported yet.
I remove some cases in dnn/test/test_onnx_conformance_layer_parser_denylist.inl.hpp as unit tests.

Related Issue

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

sl-sergei

LGTM, thank you! 👍

rogday

I think, we have to assert the kernel_size to be 1 in the constructor, if the such "pooling" doesn't actually makes sense with any other kernel size, e.g. everything you added except for min, I think. Also, could you please fix code formatting issues? E.g. add/remove spaces where necessary(in ternary operators, after if and such). Another issue is naming: max_val = min(max, val) looks odd and can be misleading.

modules/dnn/src/layers/pooling_layer.cpp

zihaomu · 2022-02-16T05:01:31Z

I think, we have to assert the kernel_size to be 1 in the constructor, if such "pooling" doesn't actually make sense with any other kernel size, e.g. everything you added except for min, I think.

Thanks for code reviewing. I don't quite understand why it is necessary to add the assert the kernel_size to be 1. Can you describe it more clearly?

modules/dnn/src/layers/pooling_layer.cpp

modules/dnn/src/onnx/onnx_importer.cpp

rogday

Thanks, looks good with a few code style issues.

modules/dnn/src/layers/pooling_layer.cpp

modules/dnn/src/onnx/onnx_importer.cpp

rogday

LGTM! 👍

alalek

BTW, Huge functions are not really maintainable. Consider avoiding extending them.

alalek · 2022-02-17T10:33:37Z

modules/dnn/src/layers/pooling_layer.cpp

+                            for (int d = dstart; d < dend; ++d) {
+                                for (int y = ystart; y < yend; ++y) {
+                                    for (int x = xstart; x < xend; ++x) {
+                                        const int index = d * inp_width * inp_height + y * inp_width + x;
+                                        float val = srcData[index];
+
+                                        switch (poolingType) {
+                                            case AVE: case SUM:


Having a switch in triple nested for loops are terrible from performance perspective.

We need dedicated "processing runners" for each type. E.g., using templates with functors.

We should not have performance regressions during implementing features.

@alalek, I think, the compiler should be able to optimize it since we're not changing poolingType, but you're right, dedicated runners make more sense.

vpisarev · 2022-02-23T09:02:46Z

@zihaomu, as we discussed, please, do the following changes:

move reduce* operations implementation into a dedicated reduce_layer.cpp
make the reduction operation a template parameter (this part you have already done) without base class and without virtual methods, because calling virtual methods inside the inner loop is terribly slow. Also, please reduce the amount of template code to the minimum. All the reduction operations share many common parts, e.g. shape inference. So, preferably only the processing loop in forward() should be made templates. In some other, non-critical, parts you may use switch operator.
in order to support minpool, please, duplicate the inner loop in maxpooling implementation. Do not place "if" inside the inner loop.

zihaomu · 2022-02-28T00:34:26Z

@zihaomu, as we discussed, please, do the following changes:

move reduce* operations implementation into a dedicated reduce_layer.cpp

make the reduction operation a template parameter (this part you have already done) without base class and without virtual methods, because calling virtual methods inside the inner loop is terribly slow. Also, please reduce the amount of template code to the minimum. All the reduction operations share many common parts, e.g. shape inference. So, preferably only the processing loop in forward() should be made templates. In some other, non-critical, parts you may use switch operator.

in order to support minpool, please, duplicate the inner loop in maxpooling implementation. Do not place "if" inside the inner loop.

Thanks for the code review, I'm refactoring the code.

alalek

Thank you for update! Overall looks good.

alalek · 2022-03-04T09:43:16Z

modules/dnn/src/layers/reduce_layer.cpp

+        bool hasSIMD = false;
+        ReduceOpBase() {}
+        virtual ~ReduceOpBase() {};
+        virtual float apply(const float*, const float*, const float ikarea = 1.0f) = 0;


Please drop inheritance from functors: alalek@pr21601_r

(with similar change for int8 implementation)

alalek · 2022-03-04T09:43:56Z

modules/dnn/src/layers/reduce_layer.cpp

+    class ReduceOpMIN : public ReduceOpBase
+    {
+    public:
+        bool hasSIMD = true;


hasSIMD

Looks like it is unused. Please remove.

alalek · 2022-03-04T09:47:34Z

modules/dnn/include/opencv2/dnn/all_layers.hpp

+    {
+    public:
+        int type;
+        std::vector<size_t> deletedDims;


deletedDims

reductionDims? or something else?

Could you please add link to some document, e.g. ONNX specification

alalek · 2022-03-04T09:48:15Z

modules/dnn/include/opencv2/dnn/all_layers.hpp

+    class CV_EXPORTS ReduceLayer : public Layer
+    {
+    public:
+        int type;


type

reduce_mode? (to avoid confusion with data type)

Thanks for your code review, all comments have been fixed.

zihaomu · 2022-03-08T07:15:38Z

Hi @alalek, Can you take a look at why the CI of Win64 OpenCL is failed? Thx.

alalek · 2022-03-08T16:01:55Z

modules/dnn/src/onnx/onnx_importer.cpp

+
+                std::swap(shouldDelete[index], shouldDelete[i]);
+                std::swap(perm[index], perm[i]);
+                std::swap(inpShape[index], inpShape[i]);


Build error from logs:

37>C:\build\precommit_opencl\4.x\opencv\modules\dnn\src\onnx\onnx_importer.cpp(1218,63): error C2665: 'std::swap': none of the 2 overloads could convert all the argument types [C:\build\precommit_opencl\build\modules\dnn\opencv_dnn.vcxproj] C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Tools\MSVC\14.27.29110\include\thread(200,13): message : could be 'void std::swap(std::thread &,std::thread &) noexcept' (compiling source file C:\build\precommit_opencl\4.x\opencv\modules\dnn\src\onnx\onnx_importer.cpp) [C:\build\precommit_opencl\build\modules\dnn\opencv_dnn.vcxproj] C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Tools\MSVC\14.27.29110\include\utility(104,19): message : or 'void std::swap<std::_Vb_reference<std::_Wrap_alloc<std::allocator<std::_Vbase>>>,0>(_Ty &,_Ty &) noexcept(<expr>)' [C:\build\precommit_opencl\build\modules\dnn\opencv_dnn.vcxproj] with [ _Ty=std::_Vb_reference<std::_Wrap_alloc<std::allocator<std::_Vbase>>> ] (compiling source file C:\build\precommit_opencl\4.x\opencv\modules\dnn\src\onnx\onnx_importer.cpp) C:\build\precommit_opencl\4.x\opencv\modules\dnn\src\onnx\onnx_importer.cpp(1218,63): message : while trying to match the argument list '(std::_Vb_reference<std::_Wrap_alloc<std::allocator<std::_Vbase>>>, std::_Vb_reference<std::_Wrap_alloc<std::allocator<std::_Vbase>>>)' [C:\build\precommit_opencl\build\modules\dnn\opencv_dnn.vcxproj]

Looks like there is no std::swap() for elements of std::vector<bool> - as there are no elements references.

Thanks a lot, I will try to solve it.

alalek · 2022-03-10T19:50:28Z

modules/dnn/src/int8layers/reduce_layer.cpp

+    public:
+        ReduceOpBase() {}
+        virtual ~ReduceOpBase() {};
+        virtual int8_t apply(const int8_t*, const int8_t*) = 0;


We still have virtual stuff.
It should be dropped as we use templates.

Thanks for the code review, I have removed all virtual stuff in the base class, both layers/reduce_layer.cpp and int8_layers/reduce_layer.cpp.

alalek

Thank you!

rogday · 2022-03-11T11:52:30Z

modules/dnn/src/int8layers/reduce_layer.cpp

+    class ReduceOpBase
+    {
+    public:
+        ReduceOpBase() {}
+        ~ReduceOpBase() {};
+        int8_t apply(const int8_t*, const int8_t*) { return 0; };
+    };


Do we even need base class now?

@rogday Thx for code reviewing, I have remove the base class, and refactor the code with struct instead of class.

zihaomu force-pushed the add_reduceLayer branch from cbc9576 to 0827ab2 Compare February 13, 2022 09:10

asmorkalov requested review from rogday and sl-sergei February 14, 2022 06:31

asmorkalov added category: dnn (onnx) ONNX suport issues in DNN module category: dnn test labels Feb 14, 2022

sl-sergei approved these changes Feb 14, 2022

View reviewed changes

rogday reviewed Feb 15, 2022

View reviewed changes

modules/dnn/src/layers/pooling_layer.cpp Outdated Show resolved Hide resolved

zihaomu commented Feb 16, 2022

View reviewed changes

modules/dnn/src/layers/pooling_layer.cpp Outdated Show resolved Hide resolved

rogday reviewed Feb 16, 2022

View reviewed changes

modules/dnn/src/onnx/onnx_importer.cpp Outdated Show resolved Hide resolved

zihaomu force-pushed the add_reduceLayer branch 2 times, most recently from 26c4325 to 3ef4302 Compare February 16, 2022 15:14

rogday reviewed Feb 16, 2022

View reviewed changes

rogday approved these changes Feb 17, 2022

View reviewed changes

alalek reviewed Feb 17, 2022

View reviewed changes

zihaomu requested review from alalek and rogday March 3, 2022 07:21

alalek reviewed Mar 4, 2022

View reviewed changes

zihaomu force-pushed the add_reduceLayer branch from 8c729af to 4d1a829 Compare March 6, 2022 01:00

alalek reviewed Mar 8, 2022

View reviewed changes

alalek requested changes Mar 10, 2022

View reviewed changes

zihaomu force-pushed the add_reduceLayer branch 2 times, most recently from 4a5b1eb to a895e3e Compare March 11, 2022 02:32

zihaomu requested a review from alalek March 11, 2022 02:37

alalek approved these changes Mar 11, 2022

View reviewed changes

rogday reviewed Mar 11, 2022

View reviewed changes

Support for some reduce layers for onnx

b6b5c27

zihaomu force-pushed the add_reduceLayer branch from e3220df to b6b5c27 Compare March 18, 2022 02:19

alalek assigned rogday Mar 18, 2022

opencv-pushbot merged commit c9b9088 into opencv:4.x Mar 26, 2022

alalek mentioned this pull request Mar 30, 2022

dnn: fix ReduceLayer implementation, update OpenVINO tests #21796

Merged

opencv-pushbot mentioned this pull request Apr 23, 2022

(5.x) Merge 4.x #21903

Merged

Uh oh!

Conversation

zihaomu commented Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The following Reduce layers of ONNX are supported:

Pull Request Readiness Checklist

Uh oh!

sl-sergei left a comment

Choose a reason for hiding this comment

Uh oh!

rogday left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zihaomu commented Feb 16, 2022

Uh oh!

Uh oh!

Uh oh!

rogday left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rogday left a comment

Choose a reason for hiding this comment

Uh oh!

alalek 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!

vpisarev commented Feb 23, 2022

Uh oh!

zihaomu commented Feb 28, 2022

Uh oh!

alalek 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zihaomu commented Mar 8, 2022

Uh oh!

alalek Mar 8, 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!

alalek 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!

Reviewers

Assignees

Labels

zihaomu commented Feb 12, 2022 •

edited

Loading

alalek Mar 8, 2022 •

edited

Loading