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] Changes in common files to enable MKL Quantized ops with native format #45107

Conversation

mahmoud-abuzaina
Copy link
Contributor

@mahmoud-abuzaina mahmoud-abuzaina commented Nov 23, 2020

This PR adds the common changes to enable MKL Quantized ops in native format mode. It removes the dependency on the duplicate number of inputs/outputs for quantized ops.

Added by @penpornk for API review:

  • The ops that are changed in API Golden files all have hidden visibility.
  • The attribute data_format is added because MKL ops support both TF's native NHWC format and its own blocked format.
  • The attributes is_weight_const and is_filter_const are added for caching purposes.
  • The changes should be backward compatible.

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! The overall PR looks good to me.
I'm a bit hesitant on adding oneDNN-specific parameters (is_*_const) to the non-MKL-prefixed Quantized ops (which are visible to C++ API users). But I'll leave this part to the API review. (Will tag them after I approve this PR.)

tensorflow/core/common_runtime/mkl_layout_pass.cc Outdated Show resolved Hide resolved
tensorflow/core/graph/mkl_graph_util.h Outdated Show resolved Hide resolved
tensorflow/core/framework/common_shape_fns.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
@mahmoud-abuzaina
Copy link
Contributor Author

Thank you for reviewing the PR. I have made the requested changed.
For adding an attribute to the ops, I thought these ops are not visible in the API as we mark them as hidden, for e.g. here. These ops usually get used by frozen graphs generated by our offline tool.

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Dec 29, 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 fixes!

For adding an attribute to the ops, I thought these ops are not visible in the API as we mark them as hidden, for e.g. here.

Oh, you're right. Sorry I forgot to check before. :)

However, this PR will still require an API review anyway because it modifies the API golden files (//tensorflow/tools/api/golden/...). Tagging API review now. (It will be reviewed next week.)

@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 29, 2020
@penpornk penpornk added the API review API Review label Dec 29, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 29, 2020
@gbaned gbaned removed the ready to pull PR ready for merge process label Dec 29, 2020
@gbaned
Copy link
Contributor

gbaned commented Dec 31, 2020

@mahmoud-abuzaina Can you please check @penpornk's comments and keep us posted ? Thanks!

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Dec 31, 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.

@mahmoud-abuzaina Sorry for the delay! The API review committee would like to see how the added attributes will be used. Can we put these attribute changes in the PRs that actually have the implementation that use them? We can keep other general changes in this PR.

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Jan 22, 2021
@penpornk penpornk removed API review API Review ready to pull PR ready for merge process labels Jan 22, 2021
@mahmoud-abuzaina
Copy link
Contributor Author

mahmoud-abuzaina commented Jan 23, 2021

@penpornk no worries. This PR and other related pending PRs don't add any new implementation to use the newly added attributes. Rather, the implementation that uses these attributes has been there for long time.
Here is the background: all Quantized* ops are hidden API ops and get generated/added to frozen pb file via offline tool. They were considered dummy ops. At runtime those ops will be rewritten to corresponding _MklQuantized*. The purpose of _MklQuantized* is to handle the extra number of inputs and outputs that we used to require. With native format support, we are getting rid of all extra inputs and outputs. So I thought we can reuse Quantized* ops (that are usable only via oneDNN integration code) instead of creating new ops. While doing that I noticed that these ops are missing few required attributes. These attributes have been used for a while with existing implementation, but the missing attributes were not causing a failure before because Quantized* ops were dummy ops and will be always rewritten to _Mkl ones which have the those attributes. Now since we are using Quantized* named ops, we are adding the missing attributes but they will use existing implementation. As an example of existing attribute usage see here. Sorry for my verbosity and please let me if I am missing something.

@gbaned gbaned added the awaiting review Pull request awaiting review label Feb 1, 2021
@mahmoud-abuzaina
Copy link
Contributor Author

Closing for now. I will do some changes and reopen once ready.

PR Queue automation moved this from Reviewer Requested Changes to Closed/Rejected Feb 5, 2021
@penpornk
Copy link
Member

penpornk commented Feb 5, 2021

@mahmoud-abuzaina So sorry for my delayed reply and thank you very much for the thorough explanation! I didn't remember that these attributes are already used in the already checked-in kernels.

Re: Hidden ops: The API review committee reminded me that op names not prefixed by _ are still callable. Marking them hidden just puts them in tf.raw_ops, so they are still part of the API golden.

Edited to add: We synced offline about closing the PR for now. Looking forward to the changes!

PR Queue automation moved this from Closed/Rejected to Assigned Reviewer Feb 13, 2021
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Feb 15, 2021
@gbaned gbaned requested a review from penpornk February 15, 2021 15:58
@gbaned gbaned added the awaiting review Pull request awaiting review label Feb 15, 2021
@gbaned gbaned requested review from penpornk and removed request for penpornk March 17, 2021 19:01
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.

I'm so sorry for the long delay. Thank you again for the PR!

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer May 20, 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 20, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 20, 2021
@gbaned gbaned removed the awaiting review Pull request awaiting review label May 21, 2021
@copybara-service copybara-service bot merged commit a2af18a into tensorflow:master May 21, 2021
PR Queue automation moved this from Approved by Reviewer to Merged May 21, 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:L CL Change Size: Large
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants