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

[Zero-Dim] support 0-D tensor for reduce/reshape/stack/prelu/expand_v2/gaussion onednn kernels #52185

Merged
merged 2 commits into from
Apr 14, 2023
Merged

[Zero-Dim] support 0-D tensor for reduce/reshape/stack/prelu/expand_v2/gaussion onednn kernels #52185

merged 2 commits into from
Apr 14, 2023

Conversation

YangQun1
Copy link
Contributor

@YangQun1 YangQun1 commented Mar 27, 2023 •
edited
Loading

PR types

New features

PR changes

OPs

Describe

For #51364
Support 0-d tensor for the following onednn kernels:

kernel 0D Usage need to be supported status
gaussian when shape=[], will output a 0D Tensor DONE
prelu x and alpha can be 0D, and output is 0D DONE
max x can be 0D, axis is [], and output is 0D DONE
min x can be 0D, axis is [], and output is 0D DONE
mean x can be 0D, axis is [], and output is 0D DONE
sum x can be 0D, axis is [], and output is 0D DONE
reshape []/[1]/[1, 1]/[1, 1, 1] can be reshape to each other DONE
stack stack multiple 0D to 1D DONE

@paddle-bot
Copy link

paddle-bot bot commented Mar 27, 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 paddle-bot bot added contributor External developers status: proposed labels Mar 27, 2023
@paddle-bot
Copy link

paddle-bot bot commented Mar 27, 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.

@YangQun1
Copy link
Contributor Author

@zhouwei25 @xinyu-intel @jczaja could you help to review? thanks

@jczaja jczaja requested review from Silv3S and jczaja March 27, 2023 10:31
@jczaja jczaja added the Intel label Mar 27, 2023
jczaja
jczaja previously approved these changes Mar 27, 2023
View reviewed changes
Copy link
Contributor

@jczaja jczaja left a comment

Choose a reason for hiding this comment

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

LGTM

xinyu-intel
xinyu-intel reviewed Mar 27, 2023
View reviewed changes
python/paddle/fluid/tests/unittests/mkldnn/test_reduce_mkldnn_op.py Outdated
Comment on lines 114 to 117
@OpTestTool.skip_if(
True,
reason="According to Paddle API, None dim means reduce all instead of copy, so just skip this test to avoid potential failure",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain 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.

According to the doc: https://www.paddlepaddle.org.cn/documentation/docs/en/api/paddle/fluid/layers/nn/reduce_sum_en.html

When dim attr is empty, reduce_sum op should reduce the input tensor to a scalar, instead of just copying the input to output without shape changing. I feel this case is an invalid case, so jsut skip it.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay.

Silv3S
Silv3S previously approved these changes Mar 28, 2023
View reviewed changes
xinyu-intel
xinyu-intel previously approved these changes Mar 30, 2023
View reviewed changes
@zhwesky2010
Copy link
Contributor

@xinyu-intel hi, in #51364, extra shape squeeze kernel is also need to support. I have added it in #51364

zhwesky2010
zhwesky2010 previously approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@zhwesky2010 zhwesky2010 left a comment

Choose a reason for hiding this comment

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

If reshape/softmax/log_softmax have add UT?

Another thing, squeeze/shape need to support 0D

@YangQun1
Copy link
Contributor Author

If reshape/softmax/log_softmax have add UT?

Another thing, squeeze/shape need to support 0D

Hi, softmax/logsoftmax are convered in #51687. I modified the table in this PR description.
For squeeze/shape, I will take a further look, thanks

@zhwesky2010
Copy link
Contributor

@YangQun1 it has been confilicted, need to be resolved.

@YangQun1 YangQun1 force-pushed the yangqun/eltwise-op-support-0d-tensor branch from 94d3509 to 8d1b70e Compare April 4, 2023 13:54
zhwesky2010
zhwesky2010 approved these changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@zhwesky2010 zhwesky2010 left a comment

Choose a reason for hiding this comment

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

LGTM

@zhwesky2010
Copy link
Contributor

zhwesky2010 commented Apr 11, 2023 •
edited
Loading

@YangQun1 hi, can it be merged? we will need it. And squeeze/shape need to support 0D

@YangQun1
Copy link
Contributor Author

@YangQun1 hi, can it be merged? we will need it. And squeeze/shape need to support 0D

this pr is ready, squeeze/shape support is WIP, will open a new PR

@xinyu-intel
Copy link
Contributor

@YuanRisheng @zhouwei25 Can you help review and merge this PR?

@zhwesky2010 zhwesky2010 merged commit 6f41e17 into PaddlePaddle:develop Apr 14, 2023
@zhwesky2010 zhwesky2010 mentioned this pull request Apr 15, 2023
Closed
@zhwesky2010
Copy link
Contributor

hi, @YangQun1 mean/sum will output 0D. When I change mean op infermeta from {1} to {}, from 1D to 0D, test_reduce_mkldnn_op will failed.

infoflow 2023-04-16 00-55-08

But #52185 has been merged. Has it not been fully supported yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhwesky2010 zhwesky2010 zhwesky2010 approved these changes

@xinyu-intel xinyu-intel xinyu-intel approved these changes

@Silv3S Silv3S Silv3S left review comments

@jczaja jczaja jczaja left review comments

Assignees

@zyfncg zyfncg

Labels
contributor External developers Intel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@YangQun1 @zhwesky2010 @xinyu-intel @jczaja @Silv3S @zyfncg