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] Migrate reduce sum+grad, mean+grad, min and max oneDNN kernels #45536

Merged
merged 23 commits into from
Sep 7, 2022
Merged

[PHI] Migrate reduce sum+grad, mean+grad, min and max oneDNN kernels #45536

merged 23 commits into from
Sep 7, 2022

Conversation

piotrekobi
Copy link
Contributor

@piotrekobi piotrekobi commented Aug 29, 2022 •
edited
Loading

PR types

Others

PR changes

Others

Describe

Migrates the following oneDNN operator kernels to PHI:

  • reduce_sum
  • reduce_sum_grad
  • reduce_mean
  • reduce_mean_grad
  • reduce_min
  • reduce_max

@paddle-bot
Copy link

paddle-bot bot commented Aug 29, 2022

你的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 Aug 29, 2022
@Silv3S Silv3S self-requested a review August 29, 2022 14:35
@Silv3S Silv3S added the Intel label Aug 29, 2022
@piotrekobi piotrekobi changed the title [PHI] Migrate reduce sum, mean, min and max oneDNN kernels [PHI] Migrate reduce sum+grad, mean+grad, min and max oneDNN kernels Aug 29, 2022
@piotrekobi
Copy link
Contributor Author

@Aganlengzi A test in the Framework CI is failing and I have no way of debugging it locally. All tests related to converting the kernels to PHI are passing locally. Can you please suggest a solution?

@Silv3S Silv3S mentioned this pull request Aug 31, 2022
Merged
@yaomichael
Copy link

@chenwhql can you help review?

@Silv3S Silv3S mentioned this pull request Sep 2, 2022
Closed
chenwhql
chenwhql reviewed Sep 5, 2022
View reviewed changes
paddle/phi/kernels/onednn/reduce_kernel.h Outdated Show resolved Hide resolved
paddle/phi/kernels/onednn/reduce_max_kernel.cc Outdated Show resolved Hide resolved
paddle/phi/kernels/onednn/reduce_max_kernel.cc Outdated Show resolved Hide resolved
paddle/phi/kernels/onednn/reduce_mean_kernel.cc Outdated Show resolved Hide resolved
paddle/phi/kernels/onednn/reduce_mean_kernel.cc Outdated Show resolved Hide resolved
paddle/phi/kernels/onednn/reduce_sum_kernel.cc Show resolved Hide resolved
@chenwhql
Copy link
Contributor

chenwhql commented Sep 5, 2022

@Aganlengzi A test in the Framework CI is failing and I have no way of debugging it locally. All tests related to converting the kernels to PHI are passing locally. Can you please suggest a solution?

We are analyzing this issue and will reply tomorrow

@chenwhql chenwhql mentioned this pull request Sep 5, 2022
Merged
@piotrekobi
Copy link
Contributor Author

@chenwhql I implemented your suggestions and merged the branch with the newest develop as well as with #45626, which is a PR that should make it easier to convert the next kernels. Please merge it before merging this PR.

chenwhql
chenwhql approved these changes Sep 6, 2022
View reviewed changes
Copy link
Contributor

@chenwhql chenwhql left a comment

Choose a reason for hiding this comment

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

LGTM

@piotrekobi
Copy link
Contributor Author

@chenwhql Could you merge this as soon as the Framework CI passes? Coverage CI is complaining about some code not being tested, but I've double checked that there are Python tests that cover it.

@piotrekobi
Copy link
Contributor Author

@jczaja Please review

@chenwhql chenwhql merged commit 2225552 into PaddlePaddle:develop Sep 7, 2022
Caozhou1995 pushed a commit to Caozhou1995/Paddle that referenced this pull request Sep 9, 2022
@piotrekobi @Silv3S @Caozhou1995
[PHI] Migrate reduce sum+grad, mean+grad, min and max oneDNN kernels (P…
b7979e8 
…addlePaddle#45536)

* gaussian random

* mkldnn to onednn renaming

* fix merge conflicts

* Migrate reduce_op oneDNN kernels to phi

* Remove unnecessary header

* remove fluid code

* onednn renaming

* Change std::vector<int64_t> to IntArray

* Fix code style

* Move classes from mkldnn_reuse.h to onednn_reuse.h

* Move more functions from mkldnn_helper.h to onednn_helpper.h

* Change MKLDNN to OneDNN in VLOG message

* Implement reviewer suggestions

Co-authored-by: Silv3S <slawomir.siwek@intel.com>
@paddle-bot-old paddle-bot-old bot removed the contributor External developers label Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenwhql chenwhql chenwhql approved these changes

@Silv3S Silv3S Awaiting requested review from Silv3S

Assignees

@Aganlengzi Aganlengzi

Labels
Intel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@piotrekobi @yaomichael @chenwhql @Aganlengzi @Silv3S