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

mkldnn directory cleanup #47779

Merged
merged 26 commits into from
Nov 15, 2022
Merged

mkldnn directory cleanup #47779

merged 26 commits into from
Nov 15, 2022

Conversation

Silv3S
Copy link
Member

@Silv3S Silv3S commented Nov 8, 2022

PR types

Others

PR changes

Others

Describe

  • second part of refactor started in matmul(v1) code clean up  #47762,
  • as kernels are migrated to PHI, some files are redundant or methods are defined in two places at same time,
  • we want to refactor remaining kernels in fluid, so it can be tested and migrated to PHI without redundancy.

@paddle-bot
Copy link

paddle-bot bot commented Nov 8, 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 Nov 8, 2022
@Silv3S Silv3S added Intel and removed contributor External developers status: proposed labels Nov 8, 2022
@paddle-bot-old paddle-bot-old bot added the contributor External developers label Nov 8, 2022
@Silv3S Silv3S requested review from jakpiase and removed request for jakpiase November 10, 2022 11:35
piotrekobi
piotrekobi reviewed Nov 10, 2022
View reviewed changes
Copy link
Contributor

@piotrekobi piotrekobi left a comment

Choose a reason for hiding this comment

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

Great job cleaning this up!
I added a few suggestions.
There's really a lot that needs to be changed after the PHI migrations... Maybe it's a good idea to Ctrl-F "MKLDNN" and change it to "OneDNN" wherever possible?

@Silv3S Silv3S mentioned this pull request Nov 10, 2022
Closed
@Silv3S
Copy link
Member Author

Silv3S commented Nov 10, 2022 •
edited
Loading

@piotrekobi TYVM for review 👍

I know that there are still many MKLDNN files, but as you can see this PR already exceeded limit of modified files. Let's try to merge what is already done. Final refactor will be done after we migrate all remaining fluid oneDNN kernels to PHI

@piotrekobi piotrekobi self-requested a review November 10, 2022 15:07
piotrekobi
piotrekobi previously approved these changes Nov 10, 2022
View reviewed changes
Copy link
Contributor

@piotrekobi piotrekobi left a comment

Choose a reason for hiding this comment

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

LGTM

YuanRisheng
YuanRisheng reviewed Nov 11, 2022
View reviewed changes
paddle/phi/kernels/funcs/selected_rows_functor.cc Outdated
@@ -371,7 +371,14 @@ add_sparse_inputs(const std::vector<const phi::SelectedRows*>& inputs,
auto& input_rows = input->rows();

#ifdef PADDLE_WITH_MKLDNN
paddle::operators::OneDNNAXPYHandler<T> axpy_handler(input_width, T(1.f));

auto& pool = paddle::platform::DeviceContextPool::Instance();
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't recommend using paddle::platform::DeviceContextPool::Instance() in PHI. You could add DeviceContext argument for this function to pass dev_ctx

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this.
This function has CPUContext instead of OneDNNContext so I'm unable to use GetDevice() method. I removed fluid dependency and created OneDNNContext before this function is called. Could you please advice if this approach 0c9ca31 is good?

Copy link
Contributor

@YuanRisheng YuanRisheng Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It seems good. LGTM

@onecatcn onecatcn requested a review from chenwhql November 14, 2022 03:48
@Silv3S Silv3S requested a review from jczaja November 14, 2022 09:04
XiaoguangHu01
XiaoguangHu01 approved these changes Nov 15, 2022
View reviewed changes
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

@YuanRisheng YuanRisheng merged commit 8a339d2 into PaddlePaddle:develop Nov 15, 2022
@Silv3S Silv3S deleted the mkldnn_cleanup branch November 15, 2022 09:03
@Silv3S Silv3S mentioned this pull request Nov 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YuanRisheng YuanRisheng YuanRisheng approved these changes

@XiaoguangHu01 XiaoguangHu01 XiaoguangHu01 approved these changes

@piotrekobi piotrekobi piotrekobi left review comments

@jakpiase jakpiase Awaiting requested review from jakpiase

@sfraczek sfraczek Awaiting requested review from sfraczek

@chenwhql chenwhql Awaiting requested review from chenwhql

@jczaja jczaja Awaiting requested review from jczaja

Assignees

@chenwhql chenwhql

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

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