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] Add MKL Conv + Bias + LeakyRelu Fusion #42856

Merged
merged 3 commits into from Sep 17, 2020
Merged

[INTEL MKL] Add MKL Conv + Bias + LeakyRelu Fusion #42856

merged 3 commits into from Sep 17, 2020

Conversation

retonym
Copy link
Contributor

@retonym retonym commented Sep 1, 2020

Add Conv + Bias + LeakyRelu fusion MKL implementation

@google-ml-butler google-ml-butler bot added the size:L CL Change Size: Large label Sep 1, 2020
@gbaned gbaned self-assigned this Sep 1, 2020
@gbaned gbaned added the comp:mkl MKL related issues label Sep 1, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Sep 1, 2020
@gbaned gbaned requested a review from penpornk September 1, 2020 11:07
@gbaned gbaned added the awaiting review Pull request awaiting review label Sep 8, 2020
penpornk
penpornk requested changes Sep 11, 2020
View reviewed changes
Copy link
Member

@penpornk penpornk left a comment •
edited

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! I have a few comments.

Note to self: Don't pull this in until #42173 is merged.

tensorflow/core/common_runtime/mkl_layout_pass_test.cc Outdated
@@ -1588,6 +1589,7 @@ REGISTER_TEST_ALL_TYPES(NodeRewrite_FusedConv2D_Positive1);
"i:1, i:1} } }" \
" attr { key: 'fused_ops' value { list: {s: 'Relu'} } }" \
" attr { key: 'epsilon' value { f: 0.001 }}" \
" attr { key: 'leakyrelu_alpha' value { f: 0.2 }}" \
Copy link
Member

Choose a reason for hiding this comment

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

Nit: would you mind moving the \ at the end to align with the rest of the lines? Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The \ are aligned.

tensorflow/core/grappler/optimizers/mkl_remapper_test.cc Outdated
Comment on lines 94 to 107
auto activate = s.WithOpName("activation");
auto fetch = s.WithOpName("fetch");
if (activation == "Relu") {
ops::Identity(fetch, ops::Relu(activate, add));
} else if (activation == "Relu6") {
ops::Identity(fetch, ops::Relu6(activate, add));
} else if (activation == "Elu") {
ops::Identity(fetch, ops::Elu(activate, add));
} else if (activation == "LeakyRelu") {
ops::Identity(fetch, ops::internal::LeakyRelu(activate, add));
} else {
ops::Identity(s.WithOpName("fetch"), add);
DCHECK(activation == "None");
ops::Identity(fetch, add);
}
Copy link
Member

Choose a reason for hiding this comment

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

The logic is the same in all three cases (AddN, AddV2, Add). Can we refactor this to a function (or a lambda function)?

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 the advice. I rewrite this part of codes.

tensorflow/core/grappler/optimizers/remapper.cc Show resolved Hide resolved
tensorflow/core/grappler/optimizers/remapper_test.cc Show resolved Hide resolved
PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Sep 11, 2020
@penpornk penpornk removed the awaiting review Pull request awaiting review label Sep 11, 2020
penpornk
penpornk approved these changes Sep 11, 2020
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 quick fixes!

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Sep 11, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 11, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 11, 2020
@penpornk penpornk removed the ready to pull PR ready for merge process label Sep 11, 2020
@penpornk
Copy link
Member

#42173 is being merged (will take a while). I'll add the ready-to-pull tag back to this PR after #42173 is done merging.

penpornk
penpornk requested changes Sep 11, 2020
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.

#42173 is merged. Could you please resolve the conflicts? Thank you!

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Sep 11, 2020
@retonym
Copy link
Contributor Author

retonym commented Sep 14, 2020

The conflicts are fixed.

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Sep 14, 2020
penpornk
penpornk approved these changes Sep 14, 2020
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 again for the PR!

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 14, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 14, 2020
@rthadur rthadur added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Sep 15, 2020
@gbaned gbaned removed the ready to pull PR ready for merge process label Sep 16, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 16, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 16, 2020
@rthadur rthadur added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Sep 16, 2020
@gbaned gbaned removed the ready to pull PR ready for merge process label Sep 17, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 17, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 17, 2020
@tensorflow-copybara tensorflow-copybara merged commit 904aed8 into tensorflow:master Sep 17, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Sep 17, 2020
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

@gbaned gbaned gbaned approved these changes

Assignees

@gbaned gbaned

Labels
cla: yes comp:mkl MKL related issues ready to pull PR ready for merge process size:L CL Change Size: Large
Projects
PR Queue
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@retonym @penpornk @gbaned @googlebot @kokoro-team @tensorflow-copybara @rthadur