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

[CPU] A new transformation that adds a convert layer if there is no reorders that support the data type conversion. #3498

Merged
merged 21 commits into from
Feb 8, 2021

Conversation

maxnick
Copy link
Contributor

@maxnick maxnick commented Dec 7, 2020

Motivation of this PR is described in the ticket #39726. The main idea is to add Convert layer if a data type conversion is required for which there is no suitable reorder case. This transformation works on the mkldnn graph optimizer level.

@maxnick maxnick added the category: CPU OpenVINO CPU plugin label Dec 7, 2020
@maxnick
Copy link
Contributor Author

maxnick commented Dec 8, 2020

@yury-intel Could you please review?

@maxnick maxnick marked this pull request as ready for review December 8, 2020 11:38
@maxnick maxnick requested review from a team and removed request for a team December 8, 2020 11:38
@dmitry-gorokhov dmitry-gorokhov added this to the 2021.3 milestone Dec 8, 2020
@maxnick
Copy link
Contributor Author

maxnick commented Dec 10, 2020

@yury-intel thanks!

@maxnick
Copy link
Contributor Author

maxnick commented Dec 10, 2020

@dmitry-gorokhov could you please review this PR?

@maxnick maxnick force-pushed the AddConvertToReorder branch 2 times, most recently from e1639f3 to e1fe187 Compare December 22, 2020 09:21
@maxnick maxnick force-pushed the AddConvertToReorder branch 2 times, most recently from 9d15d26 to d51ebad Compare December 28, 2020 08:39
@maxnick maxnick force-pushed the AddConvertToReorder branch from d51ebad to 677601e Compare December 30, 2020 09:44
@maxnick maxnick force-pushed the AddConvertToReorder branch 3 times, most recently from 7b35e35 to 0caa99e Compare January 19, 2021 14:12
@maxnick maxnick force-pushed the AddConvertToReorder branch 2 times, most recently from 4d080ef to 007851c Compare January 29, 2021 09:30
@maxnick maxnick force-pushed the AddConvertToReorder branch from cc64b40 to 30259a7 Compare February 4, 2021 11:52
Copy link
Contributor

@dmitry-gorokhov dmitry-gorokhov left a comment

Choose a reason for hiding this comment

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

In general looks good to me. Please make CI green to enable merge.

@@ -115,6 +115,41 @@ class MKLDNNGraph {
MKLDNNNodePtr InsertReorder(MKLDNNEdgePtr edge, std::string layerName, const InferenceEngine::TensorDesc& inDesc,
const InferenceEngine::TensorDesc& outDesc, bool isOptimized = false, InferenceEngine::Blob::Ptr scales = nullptr);

/**
* @brief Insert MKLDNNNode at the edge-specified location.
* This method supports two regimes. First, the node is inserted without initialization (i.e. supported descriptors initialization,
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: regimes -> mode

const auto& blockingDesc = config.inConfs[0].desc.getBlockingDesc(); // inp/out layouts must be the same
dataConfigOut.desc = TensorDesc(output->getPrecision(), input->getDims(), blockingDesc);
config.outConfs.push_back(dataConfigOut);
supportedPrimitiveDescriptors.emplace_back(config, impl_desc_type::unknown, MKLDNNMemoryDesc(config.outConfs.front().desc).getFormat());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why cannot you just do smt like: dataConfigOut.desc = dataIn.desc and then update dataConfigOut.desc precision?


class CreatorsMapFilterConstIterator;

class TensorDescCreator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things about TensorDescCreator and related logic:

  1. We definitely lack brief description of its API.
  2. It should be covered by unit tests (w/o connection to any graph patterns).

I think both tasks can be out of scope of this PR, but we should create follow up ticket.

@dmitry-gorokhov dmitry-gorokhov merged commit 7387642 into openvinotoolkit:master Feb 8, 2021
@maxnick maxnick deleted the AddConvertToReorder branch October 27, 2021 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: CPU OpenVINO CPU plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants