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

[INTEL MKL] Enable mkl group conv #50333

Merged
merged 3 commits into from
Aug 9, 2021
Merged

[INTEL MKL] Enable mkl group conv #50333

merged 3 commits into from
Aug 9, 2021

Conversation

ShengYang1
Copy link
Contributor

Group conv is already enabled in CPU. This PR enables mkl group conv.

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Jun 18, 2021
@google-cla google-cla bot added the cla: yes label Jun 18, 2021
@gbaned gbaned self-assigned this Jun 18, 2021
@gbaned gbaned added the comp:mkl MKL related issues label Jun 18, 2021
@gbaned gbaned added the awaiting review Pull request awaiting review label Jul 2, 2021
@gbaned
Copy link
Contributor

gbaned commented Jul 8, 2021

@penpornk Can you please review this PR ? Thanks!

penpornk
penpornk requested changes Jul 19, 2021
View reviewed changes
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

Thank you for the PR and sorry for the delay! I have a minor nit.

tensorflow/core/kernels/mkl/mkl_conv_ops.h
Comment on lines +245 to +250
std::vector<MKLDNN_SIZE_DTYPE> mkldnn_sizes(5, -1);
mkldnn_sizes[MKL_GROUP_FILTER_DIM_G] = group_count;
mkldnn_sizes[MKL_GROUP_FILTER_DIM_O] = filter_out_depth / group_count;
mkldnn_sizes[MKL_GROUP_FILTER_DIM_I] = filter_in_depth;
mkldnn_sizes[MKL_GROUP_FILTER_DIM_H] = filter_rows;
mkldnn_sizes[MKL_GROUP_FILTER_DIM_W] = filter_cols;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Can't we set filter_dims directly? Same for other places.
If you agree, we can just add a TODO and do it later in a separate PR (to keep this one focused on enabling group conv).

Suggested change
std::vector<MKLDNN_SIZE_DTYPE> mkldnn_sizes(5, -1);
mkldnn_sizes[MKL_GROUP_FILTER_DIM_G] = group_count;
mkldnn_sizes[MKL_GROUP_FILTER_DIM_O] = filter_out_depth / group_count;
mkldnn_sizes[MKL_GROUP_FILTER_DIM_I] = filter_in_depth;
mkldnn_sizes[MKL_GROUP_FILTER_DIM_H] = filter_rows;
mkldnn_sizes[MKL_GROUP_FILTER_DIM_W] = filter_cols;
filter_dims.resize(5);
filter_dims[MKL_GROUP_FILTER_DIM_G] = group_count;
filter_dims[MKL_GROUP_FILTER_DIM_O] = filter_out_depth / group_count;
filter_dims[MKL_GROUP_FILTER_DIM_I] = filter_in_depth;
filter_dims[MKL_GROUP_FILTER_DIM_H] = filter_rows;
filter_dims[MKL_GROUP_FILTER_DIM_W] = filter_cols;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review. Yes, we can directly set filter_dims to remove redundant vector copy. Also I have added a TODO and will submit another PR.

@penpornk penpornk removed the awaiting review Pull request awaiting review label Jul 19, 2021
penpornk
penpornk approved these changes Jul 20, 2021
View reviewed changes
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

Thank you for the change!

@rthadur rthadur added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jul 20, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 20, 2021
@rthadur rthadur added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Jul 20, 2021
@penpornk
Copy link
Member

It looks like the Linux GPU failure is related to this PR. Could you please help take a look at //tensorflow/python/kernel_tests:conv_ops_test_gpu? Thank you!

@gbaned gbaned removed the ready to pull PR ready for merge process label Jul 29, 2021
@ShengYang1
Copy link
Contributor Author

It looks like the Linux GPU failure is related to this PR. Could you please help take a look at //tensorflow/python/kernel_tests:conv_ops_test_gpu? Thank you!

Looks like Eigen conv backward doesn't support group conv, so I removed the changes for gradient test case.

@gbaned gbaned requested a review from penpornk July 29, 2021 14:17
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Jul 29, 2021
penpornk
penpornk approved these changes Jul 29, 2021
View reviewed changes
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

@penpornk penpornk removed the awaiting review Pull request awaiting review label Jul 29, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jul 29, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 29, 2021
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Aug 2, 2021
@copybara-service copybara-service bot merged commit 9676608 into tensorflow:master Aug 9, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@penpornk penpornk penpornk approved these changes

Assignees

@gbaned gbaned

Labels
cla: yes comp:mkl MKL related issues size:M CL Change Size: Medium
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ShengYang1 @gbaned @penpornk @kokoro-team @rthadur