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

[GPU] Onednn integration for handling reorders #7764

Merged
merged 1 commit into from
Oct 13, 2021
Merged

[GPU] Onednn integration for handling reorders #7764

merged 1 commit into from
Oct 13, 2021

Conversation

byungilm
Copy link
Contributor

Signed-off-by: Min, Byungil [email protected]

Details:

  • Integration for remove_redundant_reorders
  • Integration for add_required_reorders

Tickets:

  • ticket-id

@byungilm byungilm requested review from a team as code owners September 30, 2021 10:54
@openvino-pushbot openvino-pushbot added the category: GPU OpenVINO GPU plugin label Sep 30, 2021
@byungilm byungilm force-pushed the merge_remove_reorder branch from f9c4503 to c3d2150 Compare October 1, 2021 07:02
@byungilm byungilm force-pushed the merge_remove_reorder branch 2 times, most recently from aeb8e1a to 86903fd Compare October 6, 2021 03:04
isanghao
isanghao reviewed Oct 6, 2021
View reviewed changes
inference-engine/thirdparty/clDNN/src/graph_optimizer/remove_redundant_reorders.cpp
// Avoid optimization of nv12 reorder
if (node.get_dependencies().size() != 1)
continue;

bool all_users_fuse = true;
std::vector<program_node*> recalc_list;

Copy link
Contributor

Choose a reason for hiding this comment

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

I found there is change like below. Did you omit it intentionally?

@@ -318,7 +318,6 @@ void remove_redundant_reorders::run(program_impl& p) {
         auto& dep = node_ptr->get_dependency(0);
         if (!usr->is_type<quantize>() ||
             (dep.get_output_layout().format != format::b_fs_yx_fsv16 &&
-             dep.get_output_layout().format != format::fs_b_yx_fsv32 &&
              dep.get_output_layout().format != format::bfyx))
             continue;

Copy link
Contributor Author

@byungilm byungilm Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

modified

inference-engine/thirdparty/clDNN/src/graph_optimizer/remove_redundant_reorders.cpp Outdated
@@ -343,8 +348,8 @@ void remove_redundant_reorders::run(program& p) {
auto& dep = node->get_dependency(0);
if (!(usr->is_type<convolution>()) ||
(usr->get_output_layout().data_type != dep.get_output_layout().data_type) ||
(dep.get_output_layout().format != format::bfyx) ||
(usr->get_output_layout().format != format::fs_b_yx_fsv32))
(usr->get_output_layout().format != format::fs_b_yx_fsv32 && usr->get_output_layout().format != format::b_fs_yx_fsv32) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now mismatching with the comment. Could you check the comment just above this line?

Copy link
Contributor

Choose a reason for hiding this comment

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

I found that this code was initially inserted by @sshlyapn.
Sergey, do we have update in convolution code as well to support b_fs_yx_fsv32?

Copy link
Contributor

@isanghao isanghao Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

conv fusion of (b_fs_yx_fsv32 reorder) is supported only on onednn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will be fixed as discussed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is quite became quite difficult now. What about rewriting as below? It will include the diff in line 366.

        if (!(usr->is_type<convolution>()) ||
             usr->get_output_layout().data_type != dep.get_output_layout().data_type ||
             dep.get_output_layout().format != format::bfyx))
            return;
        if (usr->as<convolution().get_preferred_impl_type() == impl_types::cldnn &&
              usr->get_output_layout().format != format::fs_b_yx_fsv32)
             return;
        if (usr->as<convolution().get_preferred_impl_type() == impl_types::onednn &&
              usr->get_output_layout().format != format::b_fs_yx_fsv32)
              return;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

@byungilm byungilm requested a review from sshlyapn October 7, 2021 07:06
@byungilm byungilm force-pushed the merge_remove_reorder branch 2 times, most recently from 1256933 to bbad62c Compare October 8, 2021 01:23
isanghao
isanghao reviewed Oct 8, 2021
View reviewed changes
inference-engine/thirdparty/clDNN/src/graph_optimizer/remove_redundant_reorders.cpp Outdated
if ((usr->as<quantize>().get_preferred_impl_type() != impl_types::onednn) &&
(dep.get_output_layout().format == format::fs_b_yx_fsv32))
continue;

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I remember, we should not apply this snippet. 1) fs_b_yx_fsv32 won't be selected for onednn. 2) we should not touch cldnn logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok it will be removed.

inference-engine/thirdparty/clDNN/src/graph_optimizer/remove_redundant_reorders.cpp Outdated
@@ -343,8 +348,8 @@ void remove_redundant_reorders::run(program& p) {
auto& dep = node->get_dependency(0);
if (!(usr->is_type<convolution>()) ||
(usr->get_output_layout().data_type != dep.get_output_layout().data_type) ||
(dep.get_output_layout().format != format::bfyx) ||
(usr->get_output_layout().format != format::fs_b_yx_fsv32))
(usr->get_output_layout().format != format::fs_b_yx_fsv32 && usr->get_output_layout().format != format::b_fs_yx_fsv32) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is quite became quite difficult now. What about rewriting as below? It will include the diff in line 366.

        if (!(usr->is_type<convolution>()) ||
             usr->get_output_layout().data_type != dep.get_output_layout().data_type ||
             dep.get_output_layout().format != format::bfyx))
            return;
        if (usr->as<convolution().get_preferred_impl_type() == impl_types::cldnn &&
              usr->get_output_layout().format != format::fs_b_yx_fsv32)
             return;
        if (usr->as<convolution().get_preferred_impl_type() == impl_types::onednn &&
              usr->get_output_layout().format != format::b_fs_yx_fsv32)
              return;

@byungilm byungilm force-pushed the merge_remove_reorder branch 2 times, most recently from c0025ab to 909caa9 Compare October 8, 2021 08:33
@byungilm byungilm requested a review from isanghao October 8, 2021 09:49
@byungilm
Copy link
Contributor Author

Passed dry-run with gpu_tools/dryrun_benchmark.sh update.

@byungilm byungilm force-pushed the merge_remove_reorder branch from 909caa9 to 48cd925 Compare October 13, 2021 04:01
@isanghao isanghao merged commit d23ec24 into openvinotoolkit:master Oct 13, 2021
@vladimir-paramuzov vladimir-paramuzov added this to the 2022.1 milestone Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jade-cho jade-cho jade-cho approved these changes

@lznamens lznamens lznamens approved these changes

@isanghao isanghao isanghao approved these changes

@geunhwan geunhwan Awaiting requested review from geunhwan

@sshlyapn sshlyapn Awaiting requested review from sshlyapn

Assignees
No one assigned
Labels
category: GPU OpenVINO GPU plugin
Projects
None yet
Milestone
2022.1
Development

Successfully merging this pull request may close these issues.
6 participants
@byungilm @isanghao @lznamens @jade-cho @vladimir-paramuzov @openvino-pushbot