DickJC123 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_r253714771
##########
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:
So unfortunately, all enabled backend operator implementations must live
with the same FInPlaceOptions. I voiced concern about this restriction to
@eric-haibin-lin and he graciously put some thought into:
https://github.com/apache/incubator-mxnet/issues/13598. The issue has seen no
activity in a month, but I'd like to see it reawakened.
Until then, the code you highlight enforces that the most conservatinve
"least common denominator" FInPlaceOptions (i.e. none) are in effect for the
enabled Pooling implementations. Since an operator cannot rely on FInPlace
actually occurring, there should be no functional regression. And perf-wise, a
user must understand that at this point enabling multiple back-ends may have
some performance consequences due to the FInPlaceOption implementation.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
