[GitHub] TaoLv commented on a change in pull request #13749: Add NHWC layout support to Pooling (cpu, gpu cuda, gpu cuDNN)

[GitBox]

TaoLv commented on a change in pull request #13749: Add NHWC layout support to 
Pooling (cpu, gpu cuda, gpu cuDNN)
URL: https://github.com/apache/incubator-mxnet/pull/13749#discussion_r257109238
 
 

 ##########
 File path: src/operator/nn/mkldnn/mkldnn_pooling-inl.h
 ##########
 @@ -104,7 +104,8 @@ class MKLDNNPoolingBwd {
 inline bool SupportMKLDNNPooling(const PoolingParam &param) {
   return param.kernel.ndim() == 2 &&
          (param.pool_type == pool_enum::kMaxPooling ||
-          param.pool_type == pool_enum::kAvgPooling);
+          param.pool_type == pool_enum::kAvgPooling) &&
+         (!param.layout.has_value() || param.layout.value() == mshadow::kNCHW);
 
 Review comment:
   Yes. This change looks good to me for now. Two possible follow-ups:
   1. if layout parameter will be removed in the next major release, possibly 
we will not support NHWC.
   2. otherwise, if needed, we will extend MKL-DNN integration to support NHWC. 
Currently I don't see any necessity for it.
   
   Do you think I can put the suggestion for removing layout into 
https://github.com/apache/incubator-mxnet/issues/9686?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services