add mkldnn pooling backward #37146
add mkldnn pooling backward #37146
Conversation
[ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 756047b (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 78 times. |
zhangguanheng66
added
module: internals
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
albanD
left a comment
albanD
left a comment
There was a problem hiding this comment.
Looks good, small comments only.
| Tensor mkldnn_adaptive_avg_pool2d_backward( | ||
| const Tensor& grad_output, | ||
| const Tensor& input) { | ||
| TORCH_CHECK(input.dim() == 4, "mkldnn_adaptive_avg_pool2d_backward: Expect 2D input"); |
There was a problem hiding this comment.
nit: maybe make the error message slightly more explicit that a 4D Tensor is expected.
There was a problem hiding this comment.
@albanD, the file use many AT_ASSERTM, any difference with TORCH_CHECK? AT_ASSERTM will be deprecated, may can replace them.
There was a problem hiding this comment.
Yes you can replace all the AT_ASSERT with TORCH_CHECK
| // adjust padding until output sizes agree | ||
| bool all_equal = false; | ||
| std::vector<int64_t> output_sizes; | ||
| while (!all_equal) { |
There was a problem hiding this comment.
Given that all ops are calling here. How expensive is this? Should there be a warning in the docs that the ceil_mode might be inefficient for mkldnn Tensors?
| bool ceil_mode) { | ||
| AT_ERROR( | ||
| "mkldnn_max_pool2d: ATen not compiled with MKLDNN support"); | ||
| TORCH_CHECK(false, "mkldnn_max_pool2d: ATen not compiled with MKLDNN support"); |
There was a problem hiding this comment.
Thanks for taking the time to fix these.
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
Differential Revision: [D22440968](https://our.internmc.facebook.com/intern/diff/D22440968) [ghstack-poisoned]
Differential Revision: [D22440968](https://our.internmc.facebook.com/intern/diff/D22440968) [ghstack-poisoned]
ghstack-source-id: 962b825 Pull Request resolved: pytorch#37146
|
Hi @XiaobingSuper! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but we do not have a signature on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Stack from ghstack:
Differential Revision: D22440968