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

oneDNN kernels code cleanup #50743

Merged
merged 17 commits into from
Mar 6, 2023
Merged

oneDNN kernels code cleanup #50743

merged 17 commits into from
Mar 6, 2023

Conversation

Silv3S
Copy link
Member

@Silv3S Silv3S commented Feb 21, 2023 •
edited
Loading

PR types

Others

PR changes

Others

Describe

We used to have onednn_reuse.h which included multiple methods and utils related to oneDNN kernels. Goal is to split this header file into smaller, dedicated imports. It will improve readability and linking process. Changes done in this PR:

  • unify method signatures which differ after migrating kernels to PHI,
  • add matmul_utils.h dedicated for matmul, matmul_v2 and fused_matmul kernels,
  • rename matmul_v2_mkldnn_op.cc to matmul_mkldnn_op.cc as matmul_v2 kernel was migrated to PHI and this file has only matmul_v1 kernels left,
  • split SetOutMemDescWithLogicalLayoutFusesSupport to smaller methods, because only transpose2 can have all 3 attributes used in this method. Both fc and elementwise kernels have support just for one attribute, so it can be skipped.

@paddle-bot
Copy link

paddle-bot bot commented Feb 21, 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 Feb 21, 2023
@Silv3S Silv3S added Intel and removed contributor External developers status: proposed labels Feb 21, 2023
@paddle-bot paddle-bot bot added the contributor External developers label Feb 21, 2023
@Silv3S Silv3S requested review from tsocha and sfraczek February 23, 2023 09:09
tsocha
tsocha suggested changes Feb 23, 2023
View reviewed changes
@tsocha tsocha requested a review from jczaja February 23, 2023 10:34
tsocha
tsocha previously approved these changes Feb 23, 2023
View reviewed changes
@Silv3S
Copy link
Member Author

Silv3S commented Feb 24, 2023 •
edited
Loading

@zyfncg could you please review and approve PR-CI-APPROVAL? I just changed filename matmul_v2_mkldnn_op.cc -> matmul_mkldnn_op.cc but CI treats it as newly registered kernel.

zyfncg
zyfncg previously approved these changes Feb 24, 2023
View reviewed changes
@Silv3S
Copy link
Member Author

Silv3S commented Feb 24, 2023 •
edited
Loading

@zyfncg thank you very much for review. Could PR-CI-Coverage be manually marked? This PR didn't introduce new code, just moved methods to different directories. All unit tests are passing.

@zyfncg
Copy link
Contributor

zyfncg commented Feb 24, 2023

@zyfncg thank you very much for review. Could PR-CI-Coverage be manually marked? This PR didn't introduce new code, just moved methods to different directories. All unit tests are passing.

@XieYunshen Could you please review this PR for PR-CI-Coverage?

@Silv3S Silv3S requested a review from XieYunshen February 27, 2023 09:57
@qingqing01 qingqing01 self-requested a review February 28, 2023 02:43
@Silv3S Silv3S dismissed stale reviews from zyfncg and tsocha via a1ed12c February 28, 2023 10:44
tsocha
tsocha previously approved these changes Feb 28, 2023
View reviewed changes
@tsocha tsocha requested a review from zyfncg February 28, 2023 10:54
@Silv3S
Copy link
Member Author

Silv3S commented Feb 28, 2023

I think that problems with low coverage may occur due to our naming unification. Along with PHI migration, we started renaming MKLDNN to oneDNN. Also new unit tests now start with test_onednn_ instead of test_mkldnn_ and they were not included in ctest.

zyfncg
zyfncg previously approved these changes Feb 28, 2023
View reviewed changes
jczaja
jczaja previously approved these changes Mar 1, 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

tsocha
tsocha suggested changes Mar 1, 2023
View reviewed changes
Copy link
Contributor

@tsocha tsocha left a comment •
edited
Loading

Choose a reason for hiding this comment

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

TODOs have to be removed. resolved

tsocha
tsocha previously approved these changes Mar 1, 2023
View reviewed changes
@Silv3S Silv3S dismissed stale reviews from tsocha, jczaja, and zyfncg via 4803505 March 3, 2023 14:14
@Silv3S
Copy link
Member Author

Silv3S commented Mar 5, 2023

Problems with low coverage are resolved. @zyfncg could you approve once more and merge this PR?

@zyfncg zyfncg merged commit e205492 into PaddlePaddle:develop Mar 6, 2023
@Silv3S Silv3S deleted the onednn_ops_cleanup branch March 6, 2023 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zyfncg zyfncg zyfncg approved these changes

@jczaja jczaja jczaja left review comments

@tsocha tsocha tsocha left review comments

@sfraczek sfraczek Awaiting requested review from sfraczek

@XieYunshen XieYunshen Awaiting requested review from XieYunshen

@qingqing01 qingqing01 Awaiting requested review from qingqing01

Assignees

@zyfncg zyfncg

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

Successfully merging this pull request may close these issues.
4 participants
@Silv3S @zyfncg @jczaja @tsocha