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

[GitBox <gi...@apache.org>]

eric-haibin-lin 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_r255802003
 
 

 ##########
 File path: src/operator/nn/pooling.cc
 ##########
 @@ -421,11 +463,16 @@ NNVM_REGISTER_OP(_backward_Pooling)
 .set_attr<nnvm::FInplaceOption>(
     "FInplaceOption",
     [](const NodeAttrs &attrs) {
-#if MXNET_USE_CUDNN == 1
-  return std::vector<std::pair<int, int> >();
-#else
-  return std::vector<std::pair<int, int> >{{1, 0}};
+#if MXNET_USE_MKLDNN == 1 && MXNET_USE_CUDA == 0 && MXNET_USE_CUDNN == 0
 
 Review comment:
   mxnet-cuXX-mkl pip binary statically links cudnn and will not be affected. @szha correct me if I am wrong. 
   
   The potentially affected cases are:
   - build mxnet with mkldnn and cuda, but without cudnn 
   - build mxnet with neither mkldnn nor cuda.
   
   As this operator is used in backward for training, I see the two cases above quite rare. Therefore, I do not think this is a blocking issue for this PR. 
   
   Nonetheless, I suggest @DickJC123 create a github issue with this change so that people are aware and can be tracked later, and preferably run some performance testing to have a rough idea how big an impact it is making for the above two cases.
   
   Let's continue discussions on  #13598 to avoid such problems in the future.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services