From 5d4cb9f91c0d38ba756cfa22eaca99859d548492 Mon Sep 17 00:00:00 2001 From: Lv Tao Date: Thu, 24 May 2018 15:46:46 +0800 Subject: [PATCH 1/9] fix mkldnn pooling to support full convention --- src/operator/nn/mkldnn/mkldnn_pooling-inl.h | 17 +---------------- src/operator/nn/mkldnn/mkldnn_pooling.cc | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/src/operator/nn/mkldnn/mkldnn_pooling-inl.h b/src/operator/nn/mkldnn/mkldnn_pooling-inl.h index 4b6235ec4460..2adf76e9031b 100644 --- a/src/operator/nn/mkldnn/mkldnn_pooling-inl.h +++ b/src/operator/nn/mkldnn/mkldnn_pooling-inl.h @@ -87,23 +87,8 @@ inline bool SupportMKLDNNPooling(const PoolingParam ¶m) { inline bool SupportMKLDNNPooling(const PoolingParam ¶m, const TShape &dshape) { bool ret = SupportMKLDNNPooling(param); - if (!ret) - return false; - if (param.pooling_convention == pool_enum::kValid) - return true; - else - return false; - -// need to support pooling convention full -// https://issues.apache.org/jira/browse/MXNET-33 -#if 0 - if (((dshape[2] + 2 * param.pad[0] - param.kernel[0]) % param.stride[0] == 0) && - ((dshape[3] + 2 * param.pad[1] - param.kernel[1]) % param.stride[1] == 0)) - return true; - else - return false; -#endif + return ret; } inline bool MKLDNNRequireWorkspace(const PoolingParam ¶m) { diff --git a/src/operator/nn/mkldnn/mkldnn_pooling.cc b/src/operator/nn/mkldnn/mkldnn_pooling.cc index 259af2b94025..a65b24b79210 100644 --- a/src/operator/nn/mkldnn/mkldnn_pooling.cc +++ b/src/operator/nn/mkldnn/mkldnn_pooling.cc @@ -150,6 +150,16 @@ mkldnn::pooling_forward::primitive_desc GetPoolingFwd(const PoolingParam ¶m, int pad_l_ = param.pad[1], pad_r_ = param.pad[1]; int stride_h_ = param.stride[0], stride_w_ = param.stride[1]; + if (param.pooling_convention == pool_enum::kFull) { + if ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_ != 0) { + pad_b_ += (stride_h_ % 2) ? (stride_h_ / 2 + 1) : (stride_h_ / 2); + } + + if ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_ != 0) { + pad_r_ += (stride_w_ % 2) ? (stride_w_ / 2 + 1) : (stride_w_ / 2); + } + } + const mkldnn::engine engine = CpuEngine::Get()->get_engine(); if (param.global_pool) { pad_t_ = pad_b_ = pad_l_ = pad_r_ = 0; @@ -223,6 +233,16 @@ MKLDNNPoolingFwd &GetPoolingFwd(const PoolingParam ¶m, int pad_l_ = param.pad[1], pad_r_ = param.pad[1]; int stride_h_ = param.stride[0], stride_w_ = param.stride[1]; + if (param.pooling_convention == pool_enum::kFull) { + if ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_ != 0) { + pad_b_ += (stride_h_ % 2) ? (stride_h_ / 2 + 1) : (stride_h_ / 2); + } + + if ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_ != 0) { + pad_r_ += (stride_w_ % 2) ? (stride_w_ / 2 + 1) : (stride_w_ / 2); + } + } + if (param.global_pool) { pad_t_ = pad_b_ = pad_l_ = pad_r_ = 0; stride_h_ = stride_w_ = 1; From 093214f7d3be8bb55318ceb614906e09ae4188ea Mon Sep 17 00:00:00 2001 From: Lv Tao Date: Thu, 24 May 2018 22:31:06 +0800 Subject: [PATCH 2/9] backward with full convention --- src/operator/nn/mkldnn/mkldnn_pooling.cc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/operator/nn/mkldnn/mkldnn_pooling.cc b/src/operator/nn/mkldnn/mkldnn_pooling.cc index a65b24b79210..09e33780911f 100644 --- a/src/operator/nn/mkldnn/mkldnn_pooling.cc +++ b/src/operator/nn/mkldnn/mkldnn_pooling.cc @@ -319,6 +319,17 @@ void MKLDNNPoolingGradCompute(const OpContext &ctx, const PoolingParam ¶m, int pad_t_ = param.pad[0], pad_b_ = param.pad[0]; int pad_l_ = param.pad[1], pad_r_ = param.pad[1]; int stride_h_ = param.stride[0], stride_w_ = param.stride[1]; + + if (param.pooling_convention == pool_enum::kFull) { + if ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_ != 0) { + pad_b_ += (stride_h_ % 2) ? (stride_h_ / 2 + 1) : (stride_h_ / 2); + } + + if ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_ != 0) { + pad_r_ += (stride_w_ % 2) ? (stride_w_ / 2 + 1) : (stride_w_ / 2); + } + } + if (param.global_pool) { pad_t_ = pad_b_ = pad_l_ = pad_r_ = 0; stride_h_ = stride_w_ = 1; From 76a7d5598d62db216a619a5c316f2f67b70522d4 Mon Sep 17 00:00:00 2001 From: Lv Tao Date: Sat, 26 May 2018 00:35:19 +0800 Subject: [PATCH 3/9] fix --- src/operator/nn/mkldnn/mkldnn_pooling.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/operator/nn/mkldnn/mkldnn_pooling.cc b/src/operator/nn/mkldnn/mkldnn_pooling.cc index 09e33780911f..17ee4f6b02c3 100644 --- a/src/operator/nn/mkldnn/mkldnn_pooling.cc +++ b/src/operator/nn/mkldnn/mkldnn_pooling.cc @@ -152,11 +152,11 @@ mkldnn::pooling_forward::primitive_desc GetPoolingFwd(const PoolingParam ¶m, if (param.pooling_convention == pool_enum::kFull) { if ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_ != 0) { - pad_b_ += (stride_h_ % 2) ? (stride_h_ / 2 + 1) : (stride_h_ / 2); + pad_b_ += stride_h_ - ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_); } if ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_ != 0) { - pad_r_ += (stride_w_ % 2) ? (stride_w_ / 2 + 1) : (stride_w_ / 2); + pad_r_ += stride_w_ - ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_); } } @@ -235,11 +235,11 @@ MKLDNNPoolingFwd &GetPoolingFwd(const PoolingParam ¶m, if (param.pooling_convention == pool_enum::kFull) { if ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_ != 0) { - pad_b_ += (stride_h_ % 2) ? (stride_h_ / 2 + 1) : (stride_h_ / 2); + pad_b_ += stride_h_ - ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_); } if ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_ != 0) { - pad_r_ += (stride_w_ % 2) ? (stride_w_ / 2 + 1) : (stride_w_ / 2); + pad_r_ += stride_w_ - ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_); } } @@ -322,11 +322,11 @@ void MKLDNNPoolingGradCompute(const OpContext &ctx, const PoolingParam ¶m, if (param.pooling_convention == pool_enum::kFull) { if ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_ != 0) { - pad_b_ += (stride_h_ % 2) ? (stride_h_ / 2 + 1) : (stride_h_ / 2); + pad_b_ += stride_h_ - ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_); } if ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_ != 0) { - pad_r_ += (stride_w_ % 2) ? (stride_w_ / 2 + 1) : (stride_w_ / 2); + pad_r_ += stride_w_ - ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_); } } From e64d9cdcb67376d740fe40795ec2e9c6d2ff7c92 Mon Sep 17 00:00:00 2001 From: Lv Tao Date: Sat, 26 May 2018 22:43:52 +0800 Subject: [PATCH 4/9] add pooling test for full convention --- tests/python/gpu/test_operator_gpu.py | 29 +++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/python/gpu/test_operator_gpu.py b/tests/python/gpu/test_operator_gpu.py index d5e026262189..000f1b41527e 100644 --- a/tests/python/gpu/test_operator_gpu.py +++ b/tests/python/gpu/test_operator_gpu.py @@ -920,6 +920,35 @@ def test_3d_pooling(pool_type, p_value=2): test_3d_pooling('lp', p_value=3) +@with_seed() +def test_pooling_full_2d(): + def test_pooling_full_2d_type(pool_type): + data = (2, 2, 10, 10) + kernel = (4, 5) + pad = (1, 2) + stride = (3, 4) + + convention = 'full' + ctx_list = [] + sym_list = [] + + # o_h = ceil((10 + 1 + 1 - 4) / 3) + 1 = 4 + # o_w = ceil((10 + 2 + 2 - 5) / 4) + 1 = 4 + ctx_list.append({'ctx': mx.cpu(0), 'pool_data': data, 'type_dict': {'pool_data': np.float32}}) + sym_list.append(mx.sym.Pooling(kernel=kernel, pad=pad, stride=stride, pool_type=pool_type, + pooling_convention=convention, global_pool=True, name='pool')) + + ctx_list.append({'ctx': mx.gpu(0), 'pool_data': data, 'type_dict': {'pool_data': np.float32}}) + sym_list.append(mx.sym.Pooling(kernel=kernel, pad=pad, stride=stride, pool_type=pool_type, + pooling_convention=convention, global_pool=True, name='pool')) + + check_consistency(sym_list, ctx_list) + + test_pooling_full_2d_type('max') + test_pooling_full_2d_type('avg') + test_pooling_full_2d_type('sum') + + @with_seed() def test_global_pooling(): def test_1d_pooling(pool_type, p_value=2): From c747a96f7662e12ab57163dd305ee828d4ac46a9 Mon Sep 17 00:00:00 2001 From: Lv Tao Date: Wed, 30 May 2018 22:09:19 +0800 Subject: [PATCH 5/9] add function for computing padding size --- src/operator/nn/mkldnn/mkldnn_pooling.cc | 51 ++++++++++-------------- 1 file changed, 22 insertions(+), 29 deletions(-) diff --git a/src/operator/nn/mkldnn/mkldnn_pooling.cc b/src/operator/nn/mkldnn/mkldnn_pooling.cc index 17ee4f6b02c3..62fb14b858de 100644 --- a/src/operator/nn/mkldnn/mkldnn_pooling.cc +++ b/src/operator/nn/mkldnn/mkldnn_pooling.cc @@ -129,6 +129,14 @@ mkldnn::algorithm GetMKLDNNPoolAlgo(const PoolingParam ¶m) { } } +static inline int GetPaddingSizeFull(int x, int padl, int padr, int k, int s) { + if ((x + padl + padr - k) % s != 0) { + return (padr + s - ((x + padl + padr - k) % s)); + } else { + return padr; + } +} + mkldnn::pooling_forward::primitive_desc GetPoolingFwd(const PoolingParam ¶m, const bool is_train, const memory::desc &data_md, @@ -151,13 +159,8 @@ mkldnn::pooling_forward::primitive_desc GetPoolingFwd(const PoolingParam ¶m, int stride_h_ = param.stride[0], stride_w_ = param.stride[1]; if (param.pooling_convention == pool_enum::kFull) { - if ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_ != 0) { - pad_b_ += stride_h_ - ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_); - } - - if ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_ != 0) { - pad_r_ += stride_w_ - ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_); - } + pad_b_ = GetPaddingSizeFull(data_md.data.dims[2], pad_t_, pad_b_, kernel_h_, stride_h_); + pad_r_ = GetPaddingSizeFull(data_md.data.dims[3], pad_l_, pad_r_, kernel_w_, stride_w_); } const mkldnn::engine engine = CpuEngine::Get()->get_engine(); @@ -165,6 +168,7 @@ mkldnn::pooling_forward::primitive_desc GetPoolingFwd(const PoolingParam ¶m, pad_t_ = pad_b_ = pad_l_ = pad_r_ = 0; stride_h_ = stride_w_ = 1; } + if (pad_t_ != 0 || pad_l_ != 0) { CHECK(param.pool_type == pool_enum::kAvgPooling || param.pool_type == pool_enum::kMaxPooling) @@ -173,7 +177,6 @@ mkldnn::pooling_forward::primitive_desc GetPoolingFwd(const PoolingParam ¶m, CHECK_LT(pad_t_, kernel_h_); } - const mkldnn::algorithm alg = GetMKLDNNPoolAlgo(param); mkldnn::prop_kind kind = mkldnn::prop_kind::forward_scoring; if (is_train && alg != algorithm::pooling_avg) { @@ -234,26 +237,21 @@ MKLDNNPoolingFwd &GetPoolingFwd(const PoolingParam ¶m, int stride_h_ = param.stride[0], stride_w_ = param.stride[1]; if (param.pooling_convention == pool_enum::kFull) { - if ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_ != 0) { - pad_b_ += stride_h_ - ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_); - } - - if ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_ != 0) { - pad_r_ += stride_w_ - ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_); - } + pad_b_ = GetPaddingSizeFull(data_md.data.dims[2], pad_t_, pad_b_, kernel_h_, stride_h_); + pad_r_ = GetPaddingSizeFull(data_md.data.dims[3], pad_l_, pad_r_, kernel_w_, stride_w_); } if (param.global_pool) { - pad_t_ = pad_b_ = pad_l_ = pad_r_ = 0; - stride_h_ = stride_w_ = 1; + pad_t_ = pad_b_ = pad_l_ = pad_r_ = 0; + stride_h_ = stride_w_ = 1; } if (pad_t_ != 0 || pad_l_ != 0) { - CHECK(param.pool_type == pool_enum::kAvgPooling || - param.pool_type == pool_enum::kMaxPooling) - << "Padding implemented only for average and max pooling."; - CHECK_LT(pad_l_, kernel_w_); - CHECK_LT(pad_t_, kernel_h_); + CHECK(param.pool_type == pool_enum::kAvgPooling || + param.pool_type == pool_enum::kMaxPooling) + << "Padding implemented only for average and max pooling."; + CHECK_LT(pad_l_, kernel_w_); + CHECK_LT(pad_t_, kernel_h_); } const mkldnn::algorithm alg = GetMKLDNNPoolAlgo(param); @@ -321,13 +319,8 @@ void MKLDNNPoolingGradCompute(const OpContext &ctx, const PoolingParam ¶m, int stride_h_ = param.stride[0], stride_w_ = param.stride[1]; if (param.pooling_convention == pool_enum::kFull) { - if ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_ != 0) { - pad_b_ += stride_h_ - ((data_md.data.dims[2] + pad_t_ + pad_b_ - kernel_h_) % stride_h_); - } - - if ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_ != 0) { - pad_r_ += stride_w_ - ((data_md.data.dims[3] + pad_l_ + pad_r_ - kernel_w_) % stride_w_); - } + pad_b_ = GetPaddingSizeFull(data_md.data.dims[2], pad_t_, pad_b_, kernel_h_, stride_h_); + pad_r_ = GetPaddingSizeFull(data_md.data.dims[3], pad_l_, pad_r_, kernel_w_, stride_w_); } if (param.global_pool) { From cffdebd450fddf96251a908e36c4e754bf213a15 Mon Sep 17 00:00:00 2001 From: Lv Tao Date: Tue, 17 Jul 2018 21:36:31 +0800 Subject: [PATCH 6/9] fix unit test --- tests/python/gpu/test_operator_gpu.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/python/gpu/test_operator_gpu.py b/tests/python/gpu/test_operator_gpu.py index 72eb116ac24c..c350668b297e 100644 --- a/tests/python/gpu/test_operator_gpu.py +++ b/tests/python/gpu/test_operator_gpu.py @@ -966,11 +966,11 @@ def test_pooling_full_2d_type(pool_type): # o_w = ceil((10 + 2 + 2 - 5) / 4) + 1 = 4 ctx_list.append({'ctx': mx.cpu(0), 'pool_data': data, 'type_dict': {'pool_data': np.float32}}) sym_list.append(mx.sym.Pooling(kernel=kernel, pad=pad, stride=stride, pool_type=pool_type, - pooling_convention=convention, global_pool=True, name='pool')) + pooling_convention=convention, global_pool=False, name='pool')) ctx_list.append({'ctx': mx.gpu(0), 'pool_data': data, 'type_dict': {'pool_data': np.float32}}) sym_list.append(mx.sym.Pooling(kernel=kernel, pad=pad, stride=stride, pool_type=pool_type, - pooling_convention=convention, global_pool=True, name='pool')) + pooling_convention=convention, global_pool=False, name='pool')) check_consistency(sym_list, ctx_list) From 992ad8695d2b02cfa72d85ceed6454dd9441683b Mon Sep 17 00:00:00 2001 From: Lv Tao Date: Tue, 17 Jul 2018 22:44:33 +0800 Subject: [PATCH 7/9] only support max-pooling --- src/operator/nn/mkldnn/mkldnn_pooling-inl.h | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/operator/nn/mkldnn/mkldnn_pooling-inl.h b/src/operator/nn/mkldnn/mkldnn_pooling-inl.h index cdb051c395d2..26993376bfeb 100644 --- a/src/operator/nn/mkldnn/mkldnn_pooling-inl.h +++ b/src/operator/nn/mkldnn/mkldnn_pooling-inl.h @@ -87,8 +87,18 @@ inline bool SupportMKLDNNPooling(const PoolingParam ¶m) { inline bool SupportMKLDNNPooling(const PoolingParam ¶m, const TShape &dshape) { bool ret = SupportMKLDNNPooling(param); + if (!ret) + return false; - return ret; + if (param.pooling_convention == pool_enum::kValid) { + return true; + } else { + // currently, only max-pooling is supported for full convention + if (param.pool_type == pool_enum::kMaxPooling) + return true; + else + return false; + } } inline bool MKLDNNRequireWorkspace(const PoolingParam ¶m) { From 9f96e238c2021ead863addb05cb8c22b3d2dd4ae Mon Sep 17 00:00:00 2001 From: Lv Tao Date: Thu, 8 Nov 2018 22:41:16 +0800 Subject: [PATCH 8/9] fix pooling bwd --- src/operator/nn/mkldnn/mkldnn_pooling.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/operator/nn/mkldnn/mkldnn_pooling.cc b/src/operator/nn/mkldnn/mkldnn_pooling.cc index 437ad1b55812..18dc835c0d0b 100644 --- a/src/operator/nn/mkldnn/mkldnn_pooling.cc +++ b/src/operator/nn/mkldnn/mkldnn_pooling.cc @@ -371,6 +371,12 @@ MKLDNNPoolingBwd &GetPoolingBwd(const PoolingParam ¶m, int pad_t_ = param.pad[0], pad_b_ = param.pad[0]; int pad_l_ = param.pad[1], pad_r_ = param.pad[1]; int stride_h_ = param.stride[0], stride_w_ = param.stride[1]; + + if (param.pooling_convention == pool_enum::kFull) { + pad_b_ = GetPaddingSizeFull(data_md.data.dims[2], pad_t_, pad_b_, kernel_h_, stride_h_); + pad_r_ = GetPaddingSizeFull(data_md.data.dims[3], pad_l_, pad_r_, kernel_w_, stride_w_); + } + if (param.global_pool) { pad_t_ = pad_b_ = pad_l_ = pad_r_ = 0; stride_h_ = stride_w_ = 1; From 62fe457929ead15036efa114300f77d41db9d5cb Mon Sep 17 00:00:00 2001 From: Lv Tao Date: Fri, 16 Nov 2018 15:48:00 +0800 Subject: [PATCH 9/9] address review comment --- src/operator/nn/mkldnn/mkldnn_pooling-inl.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/operator/nn/mkldnn/mkldnn_pooling-inl.h b/src/operator/nn/mkldnn/mkldnn_pooling-inl.h index 1d283269fadd..f548778c7615 100644 --- a/src/operator/nn/mkldnn/mkldnn_pooling-inl.h +++ b/src/operator/nn/mkldnn/mkldnn_pooling-inl.h @@ -117,10 +117,7 @@ inline bool SupportMKLDNNPooling(const PoolingParam ¶m, return true; } else { // currently, only max-pooling is supported for full convention - if (param.pool_type == pool_enum::kMaxPooling) - return true; - else - return false; + return param.pool_type == pool_enum::kMaxPooling; } }