Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move Pool OPs to phi #40208

Merged
merged 8 commits into from
Mar 14, 2022
Merged

Move Pool OPs to phi #40208

merged 8 commits into from
Mar 14, 2022

Conversation

From00
Copy link
Contributor

@From00 From00 commented Mar 7, 2022

PR types

Function optimization

PR changes

OPs

Describe

  1. Move a series of Pool OPs to phi, including pool2d, pool3d, max_pool2d_with_index, and max_pool3d_with_index.
  2. Move paddle::operators::math::pooling library to phi::funcs::pooling.
  3. Add is_run_mkldnn_kernel field in MetaConfig to support checking whether running MKLDNN kernel in InferShapeFunctor.
  4. Throw a message in BuildDygraphPhiKernelContext function when the input variable is not found in NameVarMap. It is a high-frequency error and sometimes very difficult to locate when user writing typos in OpArgumentMapping function.

@paddle-bot-old
Copy link

paddle-bot-old bot commented Mar 7, 2022

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

adaptive) { // for adaptive_avg_pool2d && output_size == 1
#if defined(__HIPCC__) || defined(__NVCC__)
auto stream = ctx.stream();
funcs::TensorReduceImpl<T, T, kps::AddFunctor, kps::DivideFunctor<T>>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

该函数名已经修改为func::ReduceKernel,并且不再需要传递最后的stream参数,辛苦修改一下。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thx.

Copy link
Contributor

@YuanRisheng YuanRisheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个benchmark脚本有吗

@From00
Copy link
Contributor Author

From00 commented Mar 10, 2022

这个benchmark脚本有吗

相关的几个API添加的Benchmark脚本都已经合入了的

zyfncg
zyfncg previously approved these changes Mar 11, 2022
Comment on lines +118 to +128
void MaxPoolWithIndexGradInferMeta(const MetaTensor& x,
const MetaTensor& mask,
const MetaTensor& dout,
const std::vector<int>& kernel_size,
const std::vector<int>& strides,
const std::vector<int>& paddings,
bool global_pooling,
bool adaptive,
MetaTensor* dx) {
dx->share_meta(x);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[TODO] 这里实际上仅需要X的一个输入,但为了匹配kernel参数放入了大量无用参数,后面尝试下通过yaml配置避免这种写法

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get

Comment on lines 15 to 16
#include "paddle/phi/kernels/impl/pool_grad_kernel_impl.h"
#include "paddle/phi/kernels/pool_grad_kernel.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这两个头文件顺序换下

Copy link
Contributor Author

@From00 From00 Mar 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 15 to 16
#include "paddle/phi/kernels/impl/pool_kernel_impl.h"
#include "paddle/phi/kernels/pool_kernel.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同上


PD_REGISTER_KERNEL(
pool2d_grad, CPU, ALL_LAYOUT, phi::Pool2dGradKernel, float, double) {}
PD_REGISTER_KERNEL(pool2d_grad_grad,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pool2d_grad_grad 可以后面换成 pool2d_double_grad

Copy link
Contributor Author

@From00 From00 Mar 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

zyfncg
zyfncg previously approved these changes Mar 11, 2022
YuanRisheng
YuanRisheng previously approved these changes Mar 11, 2022
@From00 From00 dismissed stale reviews from YuanRisheng and zyfncg via 1490d2a March 12, 2022 13:32
Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ZzSean ZzSean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for op benchmark

@From00 From00 merged commit 88ec08a into PaddlePaddle:develop Mar 14, 2022
@From00 From00 deleted the move-pool-op-to-phi branch April 4, 2022 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants