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.

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] Enabled MIN_FIRST support and primitive caching for MKL-DNN Quantize OP #32486

Merged
merged 5 commits into from Nov 12, 2019
Merged

[INTEL MKL] Enabled MIN_FIRST support and primitive caching for MKL-DNN Quantize OP #32486

merged 5 commits into from Nov 12, 2019

Conversation

rgomathi
Copy link
Contributor

This PR will enable

  1. Min first mode support in the MKL-DNN quantize Op and
  2. Primitive Caching for the same Op

Code changes involve

  1. Rewriting the rules in the mkl_layout_pass.cc to allow mkl-dnn quantize function being called for MIN_FIRST mode as well
  2. Appropriate code added in mkl_quantize_op.cc to support MIN_FIRST mode and primitive caching
  3. Added 2 more tests in mkl_quantize_op_test.cc specific to MIN_FIRST mode.

@tensorflow-bot tensorflow-bot bot added the size:L CL Change Size: Large label Sep 13, 2019
@gbaned gbaned self-assigned this Sep 13, 2019
@gbaned gbaned added the comp:mkl MKL related issues label Sep 13, 2019
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Sep 13, 2019
@gbaned
Copy link
Contributor

gbaned commented Sep 18, 2019

@rgomathi Can you please resolve conflicts? Thanks!

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Sep 18, 2019
penpornk
penpornk requested changes Sep 23, 2019
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 your PR! I appreciate the detailed PR description, and I'm sorry for my delay!

tensorflow/core/graph/mkl_layout_pass.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op_test.cc Show resolved Hide resolved
PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Sep 23, 2019
@rgomathi
Copy link
Contributor Author

Hi @penpornk , Thanks for the comments, will resolve them and get back to you ASAP

@tensorflowbutler tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Sep 26, 2019
@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Oct 4, 2019
@rgomathi rgomathi force-pushed the rgomathi/mkl_quantize_minfirst branch from b52e873 to 0883191 Compare October 15, 2019 10:14
@rgomathi
Copy link
Contributor Author

Hi @penpornk , I'm extremely Sorry for the delay. Made the changes recommended. Please review and let me know your comments. Thanks.

@tensorflowbutler tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Oct 15, 2019
@gbaned gbaned added the awaiting review Pull request awaiting review label Oct 18, 2019
@gbaned gbaned requested a review from penpornk October 18, 2019 13:36
@gbaned
Copy link
Contributor

gbaned commented Oct 25, 2019

@rgomathi Can you please resolve conflicts? Thanks!

@gbaned gbaned added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Oct 25, 2019
penpornk
penpornk requested changes Oct 25, 2019
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 for the fixes and sorry for my delay! I have some more minor formatting comments and we should be good to go.

tensorflow/core/graph/mkl_layout_pass.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op_test.cc Show resolved Hide resolved
@rgomathi rgomathi force-pushed the rgomathi/mkl_quantize_minfirst branch from 0883191 to cd86332 Compare October 28, 2019 07:31
@rgomathi
Copy link
Contributor Author

Hi @penpornk , Thanks for the comments and I changed the code accordingly. Please review and let me know. Thanks

@tensorflowbutler tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Oct 28, 2019
@gbaned gbaned requested a review from penpornk October 30, 2019 09:48
@gbaned gbaned added the awaiting review Pull request awaiting review label Oct 30, 2019
@nammbash nammbash mentioned this pull request Nov 4, 2019
Merged
penpornk
penpornk requested changes Nov 5, 2019
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 fixes and I'm so sorry for my delay! I have two more minor comments.

tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
tensorflow/core/kernels/mkl_quantize_op.cc Outdated Show resolved Hide resolved
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Nov 6, 2019
@rgomathi
Copy link
Contributor Author

rgomathi commented Nov 8, 2019

Fixed them....

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Nov 8, 2019
penpornk
penpornk approved these changes Nov 8, 2019
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 your patience!

@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Nov 8, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 8, 2019
@gbaned
Copy link
Contributor

gbaned commented Nov 8, 2019

@rgomathi Could you please address Ubuntu Sanity errors? Thanks!

@gbaned gbaned added stat:awaiting response Status - Awaiting response from author kokoro:force-run Tests on submitted change and removed ready to pull PR ready for merge process labels Nov 8, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 12, 2019
@gbaned gbaned added ready to pull PR ready for merge process and removed stat:awaiting response Status - Awaiting response from author labels Nov 12, 2019
@gbaned
Copy link
Contributor

gbaned commented Nov 12, 2019

@rgomathi Never mind. Ubuntu Sanity build successful after re-trigger tests. Thanks!

tensorflow-copybara pushed a commit that referenced this pull request Nov 12, 2019
@tensorflower-gardener
Merge pull request #32486 from Intel-tensorflow:rgomathi/mkl_quantize…
d6bbe8c 
…_minfirst

PiperOrigin-RevId: 280006380
Change-Id: Iaa6feaff763eafe31bdf559b3cf4b1866dc1669b
@tensorflow-copybara tensorflow-copybara merged commit 1b8b71e into tensorflow:master Nov 12, 2019
PR Queue automation moved this from Approved by Reviewer to Merged Nov 12, 2019
@nammbash nammbash deleted the rgomathi/mkl_quantize_minfirst branch November 12, 2019 23:14
@nammbash
Copy link
Contributor

Thank you @penpornk

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 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
8 participants
@rgomathi @gbaned @nammbash @penpornk @googlebot @kokoro-team @tensorflowbutler @tensorflow-copybara