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

[Intel MKL] Enabling quantized pooling ops with native format #45484

Conversation

mahmoud-abuzaina
Copy link
Contributor

@mahmoud-abuzaina mahmoud-abuzaina commented Dec 8, 2020

No description provided.

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Dec 8, 2020
@google-cla google-cla bot added the cla: yes label Dec 8, 2020
@gbaned gbaned self-assigned this Dec 9, 2020
@gbaned gbaned added the comp:mkl MKL related issues label Dec 9, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Dec 9, 2020
@gbaned gbaned requested a review from penpornk December 9, 2020 04:55
@gbaned gbaned added the awaiting review Pull request awaiting review label Dec 21, 2020
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 so sorry for the delay! I have minor comments.

Tensor output_quantized;
conv_comp.ConvertMKL2TF<quint8>(DT_QUINT8, output, mkl_shape_tensor,
output_quantized);
if (!NativeFormatEnabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

This part can use the refactored function mentioned in #45108.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the refactored function form #45108.

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Dec 30, 2020
@penpornk penpornk removed the awaiting review Pull request awaiting review label Dec 30, 2020
@mahmoud-abuzaina
Copy link
Contributor Author

Thank you for the comments. I addressed them.

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 changes! I have one minor nit.

NativeFormatEnabled() ? output : output_quantized, output_min,
output_max);

const Tensor* mkl_shape_tensor =
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 we add _ptr to the name? Same for line 180.

Suggested change
const Tensor* mkl_shape_tensor =
const Tensor* mkl_shape_tensor_ptr =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Done.

@mahmoud-abuzaina
Copy link
Contributor Author

I have addressed the comment. Thank you!

@gbaned gbaned requested a review from penpornk January 6, 2021 13:51
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!

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Jan 7, 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 Jan 7, 2021
@penpornk penpornk removed the ready to pull PR ready for merge process label Jan 7, 2021
@penpornk
Copy link
Member

penpornk commented Jan 7, 2021

Will re-tag ready-to-pull after #45107 and #45108 are merged.

@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jan 7, 2021
@gbaned gbaned added the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Feb 5, 2021
@gbaned gbaned requested a review from penpornk February 15, 2021 14:32
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!

@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label May 20, 2021
@google-ml-butler google-ml-butler bot added the ready to pull PR ready for merge process label May 20, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 20, 2021
@gbaned gbaned removed ready to pull PR ready for merge process stat:awaiting tensorflower Status - Awaiting response from tensorflower labels May 21, 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 May 21, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 21, 2021
@copybara-service copybara-service bot merged commit dea575b into tensorflow:master May 22, 2021
PR Queue automation moved this from Approved by Reviewer to Merged May 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants