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

DepthToSpace, SpaceToDepth layers optimizations #706

Merged
merged 3 commits into from
Jun 4, 2020
Merged

DepthToSpace, SpaceToDepth layers optimizations #706

merged 3 commits into from
Jun 4, 2020

Conversation

dmitry-gorokhov
Copy link
Contributor

Issue: 31554

@dmitry-gorokhov dmitry-gorokhov requested a review from a team June 1, 2020 11:29
@dmitry-gorokhov dmitry-gorokhov requested review from a team June 1, 2020 11:29
dmitry-gorokhov
dmitry-gorokhov commented Jun 1, 2020
View reviewed changes
inference-engine/src/transformations/src/transformations/depth_to_space_fusion.cpp
@@ -91,10 +91,6 @@ void ngraph::pass::DepthToSpaceFusion::depth_to_space_fusion() {
auto reshape_after = std::make_shared<ngraph::opset3::Reshape> (permute, input3, false);

ngraph::graph_rewrite_callback callback = [this](pattern::Matcher& m) {
if (!transformation_callback(std::make_shared<ngraph::opset3::DepthToSpace>())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GlebKazantaev @itikhono Guys, now we have the same transformation_callback check in both DepthToSpace fusion and DepthToSpace decompose transformations. So from the plug-in I cannot disable only decomposition. What's the proper way to handle this?

Copy link
Contributor

Choose a reason for hiding this comment

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

current behavior: after enabling this callback you turn decomposition transformation off and turn fusion transformation on. Is it suitable for you now? I will make it possible to enable/disable each transformation independently a bit later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact there were 2 problems.

  1. CommonOptimizations pass always used default transformations callback, so I wasn't able to specify the custom one.
  2. In plug-in I uses the information regarding input rank of DepthToSpace layer (see mkldnn_plugin.cpp). That's why I moved callback call to the end of the fusion transformation to pass resulting DepthToSpace op in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GlebKazantaev @itikhono I need your approval regarding nGraph changes.

@dmitry-gorokhov dmitry-gorokhov force-pushed the feature/dmitrygo/depth_to_space branch from 6b7951c to 0099f2f Compare June 1, 2020 18:22
@dmitry-gorokhov dmitry-gorokhov requested review from a team as code owners June 1, 2020 18:22
@dmitry-gorokhov dmitry-gorokhov added the category: CPU OpenVINO CPU plugin label Jun 1, 2020
@dmitry-gorokhov dmitry-gorokhov added this to the 2020.4 milestone Jun 1, 2020
@dmitry-gorokhov dmitry-gorokhov force-pushed the feature/dmitrygo/depth_to_space branch from 0099f2f to 64dea6a Compare June 2, 2020 06:19
@dmitry-gorokhov
Copy link
Contributor Author

@openvinotoolkit/openvino-ie-cpu-developers guys, please review the PR.

@dmitry-gorokhov dmitry-gorokhov force-pushed the feature/dmitrygo/depth_to_space branch from 64dea6a to 319ff7b Compare June 2, 2020 11:51
@AlexPeskov
Copy link
Contributor

As I see from layer implementation it should support NHWC formats and 3D tensors. But func tests in this PR do not cover it. Specially I worry about NHWC implementation. Do we have a plan to improve coverage?

@dmitry-gorokhov
Copy link
Contributor Author

As I see from layer implementation it should support NHWC formats and 3D tensors. But func tests in this PR do not cover it. Specially I worry about NHWC implementation. Do we have a plan to improve coverage?

You are right:

  1. 3D was not covered because of the exception thrown from Ngraph core part. I will follow up it with the team.
  2. N[D]HWC - I tested it locally by disabling planar config. However we have to test it regularly, so I created ticket 32674 to improve our test coverage.
    Still think both items shouldn't block us from merging the PR.

@dmitry-gorokhov dmitry-gorokhov force-pushed the feature/dmitrygo/depth_to_space branch 3 times, most recently from aed11ad to 9159e0c Compare June 3, 2020 13:17
@dmitry-gorokhov dmitry-gorokhov force-pushed the feature/dmitrygo/depth_to_space branch from 9159e0c to b0dde6c Compare June 4, 2020 06:49
AlexPeskov
AlexPeskov approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@AlexPeskov AlexPeskov left a comment

Choose a reason for hiding this comment

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

LGTM

@dmitry-gorokhov dmitry-gorokhov merged commit 3183c11 into openvinotoolkit:master Jun 4, 2020
@ilya-lavrenov ilya-lavrenov added enhancement New feature or request optimization labels Jun 4, 2020
redradist pushed a commit to redradist/openvino that referenced this pull request Oct 6, 2023
@t-jankowski
Remove unused include (openvinotoolkit#706)
667b370
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itikhono itikhono itikhono approved these changes

@AlexPeskov AlexPeskov AlexPeskov approved these changes

@GlebKazantaev GlebKazantaev Awaiting requested review from GlebKazantaev

Assignees
No one assigned
Labels
category: CPU OpenVINO CPU plugin enhancement New feature or request optimization
Projects
None yet
Milestone
2020.4
Development

Successfully merging this pull request may close these issues.
4 participants
@dmitry-gorokhov @AlexPeskov @itikhono @ilya-lavrenov