[oneDNN] adaptive pool support #27747

jczaja · 2020-09-30T14:28:27Z

PR types

New features

PR changes

OPs

Describe

Added support for adaptive pooling (It was missing in oneDNN kernel) e.g. handling attribute "adaptive" and act accordingly. This is first of two fixes needed to fix #27398

test=develop

paddle-bot-old · 2020-09-30T14:28:36Z

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

jczaja · 2020-10-01T07:38:45Z

@wozna, @wojtuss , @arlesniak , @arogowie-intel Please review

arogowie-intel · 2020-10-01T10:15:20Z

paddle/fluid/platform/mkldnn_reuse.h

+        ksize[0] = static_cast<int>(
+            ceil(static_cast<double>(src_tz[src_tz.size() - 2] / ksize[0])));
+        ksize[1] = static_cast<int>(
+            ceil(static_cast<double>(src_tz[src_tz.size() - 1] / ksize[1])));


I guess you want to round up the kernel size, thus you need to cast to floating point at least one arg of division operator, otherwise both sides are integers and we have integer division. If integer division is that what you want, then cast to double is not necessary.

Suggested change

ksize[0] = static_cast<int>(

ceil(static_cast<double>(src_tz[src_tz.size() - 2] / ksize[0])));

ksize[1] = static_cast<int>(

ceil(static_cast<double>(src_tz[src_tz.size() - 1] / ksize[1])));

ksize[0] = static_cast<int>(

ceil(static_cast<double>(src_tz[src_tz.size() - 2]) / ksize[0]));

ksize[1] = static_cast<int>(

ceil(static_cast<double>(src_tz[src_tz.size() - 1]) / ksize[1]));

I'd suggest to have UT which distinguish those two slightly different rounding cases.

@arogowie-intel After some analysis of reference solution I restricted supported cases only to those when src_tz dims are divisible by corressponding ksize dims. This is because when we have some rounding applied then we may end up that during one pooling operation (multiple poolings over same input) pooling window size may vary , which is not supported by oneDNN.

arlesniak · 2020-10-01T12:07:11Z

python/paddle/fluid/tests/unittests/mkldnn/test_pool2d_mkldnn_op.py

+
+    def init_test_case(self):
+        self.ksize = [1, 1]
+        self.strides = [1, 1]


I'm wondering if you could put test case triggering another calculation in additon to happy path in the mkldnn_reuse when adaptive is used.

@arlesniak I have added another test. Thanks for pointing out missing scenario coverage

test=develop

arlesniak

LGTM

jczaja · 2020-10-06T17:07:52Z

@luotao1 Could you please start your review?

lidanqing-intel · 2020-10-06T21:11:42Z

paddle/fluid/platform/mkldnn_reuse.h

@@ -853,6 +853,9 @@ class PoolingMKLDNNHandler : public MKLDNNHandlerT<T, mkldnn::pooling_forward,
        CorrectOutputSize(src_tz, dst_tz, ksize, paddings, strides,
                          mkldnn_paddings[1]);
      }
+


why ctx.Attr<bool>("adaptive") is not here but inside the function ComputeAdaptivePoolParameters?

In the function ComputeAdaptivePoolParameters declaration, it is not clear what will be changed, what will not. Maybe we could refer to tester_helper.cc

void TestOneThreadPrediction(const PaddlePredictor::Config *config, const std::vector<std::vector<PaddleTensor>> &inputs, std::vector<std::vector<PaddleTensor>> *outputs, int num_threads, bool use_analysis = FLAGS_use_analysis)

in this way from the ComputeAdaptivePoolParameters(ctx, src_tz, &ksize, &strides);, we know what will be changed what will not.
But maybe point 2 is not important

ComputeAdaptivePoolParameters is used in two places e.g. once inside PoolMKLDNNHandler and once directly in PoolGrad. So it was easiest for me to just read it inside rather than read it in two places and then pass as argument which would introduced tiny overhead as more data is passed to call.

Ok, it is clear. Thanks

lidanqing-intel · 2020-10-06T22:31:40Z

paddle/fluid/operators/mkldnn/pool_mkldnn_op.cc

@@ -126,6 +126,9 @@ class PoolMKLDNNGradOpKernel : public paddle::framework::OpKernel<T> {
    UpdatePadding(&paddings, global_pooling, 0, padding_algorithm, data_dims,
                  strides, ksize);

+    platform::PoolingMKLDNNHandler<T>::ComputeAdaptivePoolParameters(


Should UpdatePadding(&paddings, global_pooling, 0, padding_algorithm, data_dims,strides, ksize); be changed to
UpdatePadding(&paddings, global_pooling, ctx.Attr<bool>("adaptive"), padding_algorithm, data_dims,strides, ksize);, or it does not matter ?

@lidanqing-intel I think they should stay seprate as UpdatePadding is a common function used for CPU and oneDNN kernels (perhaps GPU as well) and computeAdaptivePoolParameters is just aplicable to oneDNN kernel.

jczaja · 2020-10-09T10:26:08Z

@luotao1 Could you please start your review?

luotao1 · 2020-10-10T02:10:20Z

python/paddle/fluid/tests/unittests/mkldnn/test_pool2d_mkldnn_op.py

@@ -160,4 +191,6 @@ def init_shape(self):


 if __name__ == '__main__':
+    from paddle import enable_static
+    enable_static()


Why use enable_static only in this mkldnn unit-test?

@luotao1 It should be in every test , it just I was working on that one so I added only in this one.

luotao1

LGTM

jczaja added 7 commits September 24, 2020 17:08

- initial adaptive pool support

b5753a4

- compilation fix

624fb72

- fix

530597b

- implemented adaptive pooling for oneDNN pool kernel

44ee83e

- compilation fix

e5c4e15

- Added UT for adaptive pooling

3ea29a8

- Added UT to adaptive pool2d of oneDNN

d10e94c

test=develop

jczaja added the Intel label Sep 30, 2020

jczaja requested review from wojtuss and wozna September 30, 2020 16:29

jczaja assigned luotao1 Oct 1, 2020

arogowie-intel reviewed Oct 1, 2020

View reviewed changes

arlesniak reviewed Oct 1, 2020

View reviewed changes

jczaja added 8 commits October 5, 2020 18:19

- fix to UT

65278ba

test=develop

- fix

eac1c09

- grad onednn pool adaptive

d427452

- fix

dd5152c

test=develop

- compilation fix

024f910

test=develop

- another compilation fix

a0c34b7

test=develop

- antoher compilation fix

abb6daf

test=develop

- disabled non-linear adaptive pooling for oneDNN kernels

b7396f6

test=develop

arlesniak approved these changes Oct 6, 2020

View reviewed changes

lidanqing-intel reviewed Oct 6, 2020

View reviewed changes

jczaja closed this Oct 7, 2020

jczaja reopened this Oct 7, 2020

jczaja closed this Oct 8, 2020

jczaja reopened this Oct 8, 2020

jczaja mentioned this pull request Oct 9, 2020

rec_r34_vd_tps_bilstm_attn(from PaddleOCR repo) MKLDNN prediction failed #27398

Closed

luotao1 reviewed Oct 10, 2020

View reviewed changes

luotao1 approved these changes Oct 12, 2020

View reviewed changes

luotao1 merged commit 55e6376 into PaddlePaddle:develop Oct 12, 2020

chen-zhiyu pushed a commit to chen-zhiyu/Paddle that referenced this pull request Oct 15, 2020

[oneDNN] adaptive pool support (PaddlePaddle#27747)

463c033

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[oneDNN] adaptive pool support #27747

[oneDNN] adaptive pool support #27747

jczaja commented Sep 30, 2020 •

edited

Loading

paddle-bot-old bot commented Sep 30, 2020

jczaja commented Oct 1, 2020

arogowie-intel Oct 1, 2020

arogowie-intel Oct 1, 2020

jczaja Oct 6, 2020

arlesniak Oct 1, 2020

jczaja Oct 6, 2020

arlesniak left a comment

jczaja commented Oct 6, 2020

lidanqing-intel Oct 6, 2020

jczaja Oct 7, 2020

lidanqing-intel Oct 7, 2020

lidanqing-intel Oct 6, 2020

jczaja Oct 7, 2020

jczaja commented Oct 9, 2020

luotao1 Oct 10, 2020

jczaja Oct 12, 2020

luotao1 Oct 12, 2020

luotao1 left a comment

[oneDNN] adaptive pool support #27747

[oneDNN] adaptive pool support #27747

Conversation

jczaja commented Sep 30, 2020 • edited Loading

PR types

PR changes

Describe

paddle-bot-old bot commented Sep 30, 2020

jczaja commented Oct 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlesniak left a comment

Choose a reason for hiding this comment

jczaja commented Oct 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jczaja commented Oct 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luotao1 left a comment

Choose a reason for hiding this comment

jczaja commented Sep 30, 2020 •

edited

Loading