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.

Sign up for GitHub

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

Jump to bottom

[PHI kernels] Bind XPU kernels #53336

Merged
merged 2 commits into from
May 9, 2023
Merged

[PHI kernels] Bind XPU kernels #53336

merged 2 commits into from
May 9, 2023

Conversation

RuohengMa
Copy link
Contributor

@RuohengMa RuohengMa commented Apr 25, 2023 •
edited
Loading

PR types

Others

PR changes

OPs

Description

Bind sparse_coo_tensor, reduce_max/max_int32, range/arange_int32, equal_bool, scatter_grad_float32, nearest_interp_int64 kernels

@paddle-bot
Copy link

paddle-bot bot commented Apr 25, 2023

你的PR提交成功，感谢你对开源项目的贡献!
请关注后续CI自动化测试结果，详情请参考Paddle-CI手册。
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot
Copy link

paddle-bot bot commented Apr 25, 2023

❌ The PR is not created using PR's template. You can refer to this Demo.
Please use PR's template, it helps save our maintainers' time so that more developers get helped.

@RuohengMa RuohengMa force-pushed the bind branch 4 times, most recently from 340f508 to fe7ccaa Compare April 26, 2023 07:31
taixiurong
taixiurong reviewed Apr 27, 2023
View reviewed changes
paddle/phi/kernels/xpu/scatter_grad_kernel.cc Outdated
int r;
if (index_type == phi::DataType::INT32) {
auto index_data = const_cast<int *>(index.data<int>());
xpu::VectorParam<int> indices{
Copy link
Contributor

Choose a reason for hiding this comment

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

这里为啥要把const去掉？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

因为xdnn里的VectorParam里的xpu数据需要非const的指针~

@RuohengMa RuohengMa force-pushed the bind branch 4 times, most recently from 7dd470e to a4a6765 Compare April 28, 2023 09:09
luotao1
luotao1 previously approved these changes May 4, 2023
View reviewed changes
Copy link
Contributor

@luotao1 luotao1 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 const_cast

XiaociZhang
XiaociZhang suggested changes May 4, 2023
View reviewed changes
paddle/phi/kernels/sparse/cpu/sparse_utils_kernel.cc Outdated
@@ -434,6 +434,7 @@ PD_REGISTER_KERNEL(values_csr,
kernel->InputAt(0).SetDataLayout(phi::DataLayout::SPARSE_CSR);
}

#if !defined(PADDLE_WITH_XPU)
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

Choose a reason for hiding this comment

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

因为sparse下面，cpu和xpu的sparse_coo_tensor都是绑定的同一个SparseCooTensorKernel，如果同时编译的话会产生重复绑定报错。另外因为这个kernel可以根据输入Backend产生一个包含了两个dense_tensors的coo_tensor，所以cpu和xpu可以共用一个kernel~

Copy link
Contributor

Choose a reason for hiding this comment

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

cpu和xpu的sparse_coo_tensor都是绑定的同一个SparseCooTensorKernel，如果同时编译的话会产生重复绑定报错

感觉不太合理啊，别的kernel之前好像没有类似问题？

paddle/phi/backends/xpu/xpu2_op_list.cc
@@ -552,7 +558,8 @@ XPUOpMap& get_kl2_ops() {
{"reciprocal_grad",
XPUKernelSet({phi::DataType::FLOAT32, phi::DataType::FLOAT16})},
{"reduce_max_grad", XPUKernelSet({phi::DataType::FLOAT32})},
{"reduce_max", XPUKernelSet({phi::DataType::FLOAT32})},
{"reduce_max",
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

Choose a reason for hiding this comment

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

ok

paddle/phi/backends/xpu/xpu2_op_list.cc
@@ -668,6 +677,14 @@ XPUOpMap& get_kl2_ops() {
XPUKernelSet({phi::DataType::FLOAT32, phi::DataType::FLOAT16})},
{"softplus", XPUKernelSet({phi::DataType::FLOAT32})},
{"softplus_grad", XPUKernelSet({phi::DataType::FLOAT32})},
{"sparse_coo_tensor",
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

Choose a reason for hiding this comment

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

这个sparse_coo_tensor是根据Backend产生不同的coo_tensor，但不会对数据进行处理，这样的话还需要单测吗？

Copy link
Contributor

Choose a reason for hiding this comment

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

python/paddle/fluid/tests/unittests/test_sparse_utils_op.py
cpu/gpu的，可以参考

@RuohengMa
bind sparse_coo_tensor, reduce_max/max_int32, range/arange_int32, equ…
a5ae947 
…al_bool, scatter_grad_float32, nearest_interp_int64 kernels
@paddle-ci-bot
Copy link

paddle-ci-bot bot commented May 6, 2023

Sorry to inform you that a4a6765's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually.

ZibinGuo
ZibinGuo previously approved these changes May 6, 2023
View reviewed changes
Copy link
Contributor

@ZibinGuo ZibinGuo left a comment

Choose a reason for hiding this comment

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

LGTM

@RuohengMa RuohengMa dismissed stale reviews from ZibinGuo and luotao1 via d58ad27 May 8, 2023 04:34
ykkk2333
ykkk2333 approved these changes May 8, 2023
View reviewed changes
Copy link
Contributor

@ykkk2333 ykkk2333 left a comment

Choose a reason for hiding this comment

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

LGTM

paddle/phi/kernels/xpu/scatter_grad_kernel.cc Show resolved Hide resolved
luotao1
luotao1 approved these changes May 8, 2023
View reviewed changes
Copy link
Contributor

@luotao1 luotao1 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 const_cast

@houj04 houj04 merged commit 7e9c87c into PaddlePaddle:develop May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XiaociZhang XiaociZhang XiaociZhang requested changes

@taixiurong taixiurong taixiurong left review comments

@ykkk2333 ykkk2333 ykkk2333 approved these changes

@houj04 houj04 houj04 approved these changes

@luotao1 luotao1 luotao1 approved these changes

@ZibinGuo ZibinGuo ZibinGuo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@RuohengMa @taixiurong @luotao1 @houj04 @ZibinGuo @ykkk2333 @XiaociZhang