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 MatMul + Bias + LeakyRelu fusion #45031

Merged
merged 6 commits into from Dec 29, 2020
Merged

[INTEL_MKL] Enable MatMul + Bias + LeakyRelu fusion #45031

merged 6 commits into from Dec 29, 2020

Conversation

retonym
Copy link
Contributor

@retonym retonym commented Nov 20, 2020

This PR enables MatMul + Bias + LeakyRelu fusion with both MKL and Eigen implementation

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Nov 20, 2020
@google-cla google-cla bot added the cla: yes label Nov 20, 2020
@gbaned gbaned self-assigned this Nov 20, 2020
@gbaned gbaned added the comp:mkl MKL related issues label Nov 20, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Nov 20, 2020
@gbaned gbaned requested a review from penpornk November 20, 2020 13:29
@gbaned gbaned added the awaiting review Pull request awaiting review label Nov 25, 2020
@retonym retonym force-pushed the yunfeimao/matmul_leakyrelu_fusion branch from 7ee7621 to dfb45cd Compare December 21, 2020 10:02
@retonym
Copy link
Contributor Author

retonym commented Dec 21, 2020

Merge conflict is resolved. Please review this PR. Thanks

penpornk
penpornk requested changes Dec 23, 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 PR and I'm sorry for the delay! I have a few minor comments.

tensorflow/core/grappler/optimizers/remapper.cc Outdated Show resolved Hide resolved
tensorflow/core/grappler/optimizers/remapper_test.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/matmul_op_fused.cc Outdated Show resolved Hide resolved
tensorflow/core/ops/math_ops.cc Outdated Show resolved Hide resolved
PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Dec 23, 2020
@penpornk penpornk removed the awaiting review Pull request awaiting review label Dec 23, 2020
@retonym
Copy link
Contributor Author

retonym commented Dec 23, 2020

Thanks for the review. The code is changed based on comments.

penpornk
penpornk reviewed Dec 23, 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 fixes! The PR looks good to me now.
@ezhulenev Could you please help take a look if the grappler parts are okay? I'll defer the approval to you.

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 24, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 24, 2020
penpornk
penpornk approved these changes Dec 24, 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, Eugene!
(I'm approving too just so the PR status doesn't keep saying "changes requested".)

@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label Dec 24, 2020
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Dec 24, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 24, 2020
penpornk
penpornk requested changes Dec 24, 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.

@retonym Could you please look into the remapper test failures in Ubuntu CPU and Linux GPU CIs? Thank you!

[ RUN      ] RemapperFuseMatMulWithBiasAndActivationTest.F32
tensorflow/core/grappler/optimizers/remapper_test.cc:699: Failure
Expected equality of these values:
  node.op()
    Which is: "Tanh"
  "_FusedMatMul"
tensorflow/core/grappler/optimizers/remapper_test.cc:700: Failure
Expected: (node.input_size()) >= (3), actual: 1 vs 3
[  FAILED  ] RemapperFuseMatMulWithBiasAndActivationTest.F32 (11 ms)

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Dec 24, 2020
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Dec 25, 2020
@retonym
Copy link
Contributor Author

retonym commented Dec 25, 2020

The UT failure is fixed.
MatMul+Bias+Tanh fusion is enabled in MKL only currently. It should not be tested in RemapperTest with normal Tensorflow.
Merry Christmas.

penpornk
penpornk approved these changes Dec 25, 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 fix! Merry Christmas and happy holidays to you too! :)

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 25, 2020
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Dec 25, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 25, 2020
@copybara-service copybara-service bot merged commit 296d953 into tensorflow:master Dec 29, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ezhulenev ezhulenev ezhulenev approved these changes

@penpornk penpornk penpornk approved these changes

Assignees

@gbaned gbaned

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@retonym @ezhulenev @penpornk @kokoro-team @gbaned