[Transformations] Added interchangeable reshape elimination by a-sidorova · Pull Request #9691 · openvinotoolkit/openvino

a-sidorova · 2022-01-17T06:38:12Z

Details:

Updated transformation to eliminate interchangeable reshape nodes (Reshape, Squeeze, Unsqueeze).
Example:

Tickets:

CVS-70185

dmitry-gorokhov · 2022-01-19T07:52:04Z

@v-Golubev please review the PR
@GlebKazantaev would be great if you could take a look as well

src/common/transformations/src/transformations/common_optimizations/nop_elimination.cpp

GlebKazantaev · 2022-01-25T09:43:41Z

src/common/transformations/src/transformations/common_optimizations/nop_elimination.cpp

-    auto input_node = input.get_node_shared_ptr();
-    if (ov::as_type_ptr<opset3::Squeeze>(input_node) ||
-        ov::as_type_ptr<opset3::Unsqueeze>(input_node) ||
-        ov::as_type_ptr<opset3::Reshape>(input_node)) {


This check wasn't precise as we don't check the number of consumers here, so we potentially could ruin the model.

I added check for consumer count in this transformation

GlebKazantaev · 2022-01-25T10:55:48Z

src/common/transformations/src/transformations/common_optimizations/nop_elimination.cpp

+        auto input_node = input.get_node_shared_ptr();
+        if (ov::as_type_ptr<opset3::Squeeze>(input_node) ||
+            ov::as_type_ptr<opset3::Unsqueeze>(input_node) ||
+            ov::as_type_ptr<opset3::Reshape>(input_node)) {


We have dedicated transformation ReshapeSequenceFusion which performs such type of elimination, and this pass doesn't perform this fusion correctly in case of Reshape->Reshape pattern. So I suggest to remove Reshape from this condition.

Also we have to check that number of consumer is equal to 1.

I added check for consumer count in this transformation and removed Reshape from the condition.
But as I understand, transformation ReshapeSequenceFusion doesn't eliminate redundant reshapes, it always fuses into one reshape. So I updated ReshapeSequenceFusion transformation to cover interchangeable reshapes (to eliminate it)
Thanks!

Now I'm not sure about Reshape removing from this condition because transformation NopElimination contains EliminationSqueeze and EliminationUnsqueeze that convert Squeeze and Unsqueeze to Reshape. Moreover NopElimination contains EliminationReshape. And if we remove Reshape, using NopElimination we will have pattern Reshape->Reshape after Squeeze->Reshape, for example. Is it ok for NopElimination? What do you think?

GlebKazantaev · 2022-01-25T10:57:22Z

src/plugins/intel_cpu/src/ngraph_transformations/convert_to_cpu_specific_opset.hpp

    if (!ngraph::op::util::has_op_with_type<ngraph::op::FakeQuantize>(nGraphFunc)) {
        manager.register_pass<ReshapeFullyConnectedFusion>();
    }
+    manager.register_pass<ngraph::pass::EliminateReshape>();


If some transformation inserts such reshapes, so you can simply call this EliminateReshape().apply(reshape) from this pass.

GlebKazantaev · 2022-01-25T11:01:45Z

src/tests/functional/inference_engine/transformations/nop_elimination.cpp

+    ASSERT_TRUE(movement_are_missing);
+}
+
+TEST(nop_elimination, squeeze_unsqueeze_elimination) {


I don't insist but in most tests we use TEST_F(TransformationTestsF fixture, so we don't have to check everything manually.

This file contains only TEST(nop_elimination) so I created test using this fixture. But I decided to create new test for updated transformation ReshapeSequenceFusion (because I added elimination support for redundant reorders) and I used TEST_F(TransformationTestsF fixture for it. Thanks!

GlebKazantaev · 2022-01-28T08:51:53Z

src/common/transformations/src/transformations/common_optimizations/nop_elimination.cpp

+
    // eliminate redundant reshape, squeeze, or unsqueeze
    auto input_node = input.get_node_shared_ptr();
+    if (input_node->get_output_size() != 1)


It doesn't check the number of consumers, as get_output_size returns number of output ports but not consumers. Please use get_target_inputs after you casted input node to one of this reshape like operations.

And please add a tests for this.

I got it, thank you very much!
Added get_output_target_inputs and added test reshape_elimination_v1_check_consumer_count for this case.

GlebKazantaev · 2022-01-28T09:08:16Z

src/common/transformations/src/transformations/common_optimizations/reshape_sequence_fusion.cpp

+        if (input.get_node_shared_ptr()->get_output_partial_shape(0).is_static() && reshape->get_output_partial_shape(0).is_static() &&
+            input.get_node_shared_ptr()->get_output_shape(0) == reshape->get_output_shape(0)) {
+            return replace_output_update_name(reshape->output(0), input);
+        } else {
+            reshape->input(0).replace_source_output(input);
+            copy_runtime_info(nodes, reshape);
+        }


Input is already an Output class so you can get partial shape from it.
Also as this transformation is included into MOC pipeline where we can rely on shapes only in case if use_shape is true, so we have to parametrize transformation (for example: https://github.com/openvinotoolkit/openvino/blob/master/src/common/transformations/include/transformations/common_optimizations/split_concat_pair_to_interpolate_fusion.hpp#L32 )

Suggested change

if (input.get_node_shared_ptr()->get_output_partial_shape(0).is_static() && reshape->get_output_partial_shape(0).is_static() &&

input.get_node_shared_ptr()->get_output_shape(0) == reshape->get_output_shape(0)) {

return replace_output_update_name(reshape->output(0), input);

} else {

reshape->input(0).replace_source_output(input);

copy_runtime_info(nodes, reshape);

}

bool replaced = false;

if (use_shapes && input.get_partial_shape().is_static() && reshape->get_output_partial_shape(0).is_static() &&

input.get_shape() == reshape->get_output_shape(0)) {

// in case if elimination is not allowed we still can eliminate all transposes except last one

replaced = replace_output_update_name(reshape->output(0), input));

}

if (!replaced) {

reshape->input(0).replace_source_output(input);

copy_runtime_info(nodes, reshape);

}

Also please add tests for this case.

Thank you for the explanation!
Done.
About test - Before I added test ReshapeSequenceFusionEliminate that covers case when we can remove 2 redundant reorders. I think it's enough

a-sidorova · 2022-02-02T09:56:29Z

@GlebKazantaev take a look one more time please

GlebKazantaev · 2022-02-08T17:46:22Z

src/common/transformations/src/transformations/common_optimizations/reshape_sequence_fusion.cpp

+            copy_runtime_info(nodes, reshape);
+        }
+
        return false;


Suggested change

return false;

return true;

GlebKazantaev · 2022-02-08T17:46:55Z

src/common/transformations/src/transformations/common_optimizations/reshape_sequence_fusion.cpp

+
+        if (!replaced) {
+            reshape->input(0).replace_source_output(input);
+            copy_runtime_info(nodes, reshape);


Suggested change

copy_runtime_info(nodes, reshape);

copy_runtime_info(nodes, reshape);

return false; // because root node wasn't replaced

Done, thank you

GlebKazantaev · 2022-02-08T17:54:10Z

...mon/transformations/include/transformations/common_optimizations/reshape_sequence_fusion.hpp

 public:
    NGRAPH_RTTI_DECLARATION;
-    ReshapeSequenceFusion();
+    ReshapeSequenceFusion(bool use_shape_for_elimination = true);


Please update moc_transformations.cpp
https://github.com/openvinotoolkit/openvino/blob/master/src/common/transformations/src/transformations/common_optimizations/moc_transformations.cpp#L156

common_fusions->add_matcher<ngraph::pass::ReshapeSequenceFusion>(m_use_shapes);

Sure, thanks

GlebKazantaev · 2022-02-08T18:18:42Z

In general LGTM, just some small comments remaining and we can merge.

a-sidorova requested review from a team January 17, 2022 06:38

openvino-pushbot added category: CPU OpenVINO CPU plugin category: inference OpenVINO Runtime library - Inference category: IE Tests OpenVINO Test: plugins and common category: Core OpenVINO Core (aka ngraph) labels Jan 17, 2022

a-sidorova added this to the 2022.1 milestone Jan 17, 2022

dmitry-gorokhov assigned v-Golubev Jan 19, 2022

v-Golubev reviewed Jan 20, 2022

View reviewed changes

src/common/transformations/src/transformations/common_optimizations/nop_elimination.cpp Outdated Show resolved Hide resolved

a-sidorova force-pushed the feature/transformations/reshape_elimination branch from f3fb719 to 53ae67c Compare January 20, 2022 17:11

v-Golubev approved these changes Jan 21, 2022

View reviewed changes

GlebKazantaev self-assigned this Jan 25, 2022

GlebKazantaev reviewed Jan 25, 2022

View reviewed changes

a-sidorova force-pushed the feature/transformations/reshape_elimination branch from 53ae67c to 43a5903 Compare January 26, 2022 07:23

a-sidorova requested a review from GlebKazantaev January 26, 2022 11:10

GlebKazantaev reviewed Jan 28, 2022

View reviewed changes

a-sidorova force-pushed the feature/transformations/reshape_elimination branch from 7bace72 to 365386b Compare January 28, 2022 15:57

a-sidorova requested a review from GlebKazantaev January 28, 2022 20:26

a-sidorova force-pushed the feature/transformations/reshape_elimination branch 2 times, most recently from 5c92007 to ab1ffc4 Compare February 2, 2022 08:42

a-sidorova force-pushed the feature/transformations/reshape_elimination branch from ab1ffc4 to 642822f Compare February 7, 2022 06:25

GlebKazantaev reviewed Feb 8, 2022

View reviewed changes

a-sidorova added 5 commits February 9, 2022 09:20

[Transformations] Added interchangeable reshape elimination

db3c2ce

Applied comments #2

d901b15

returned Reshape in condition

597375d

applied comments #3

153fd5c

applied comments #4

e81d505

a-sidorova force-pushed the feature/transformations/reshape_elimination branch from 642822f to e81d505 Compare February 9, 2022 06:51

added comment in plugin with reason about transformation

d25ddb8

a-sidorova requested a review from GlebKazantaev February 9, 2022 16:14

GlebKazantaev approved these changes Feb 9, 2022

View reviewed changes

GlebKazantaev merged commit fce49e6 into openvinotoolkit:master Feb 9, 2022

	copy_runtime_info(nodes, reshape);
	copy_runtime_info(nodes, reshape);
	return false; // because root node wasn't replaced

Conversation

a-sidorova commented Jan 17, 2022

Details:

Tickets:

Uh oh!

dmitry-gorokhov commented Jan 19, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-sidorova commented Feb 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GlebKazantaev Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GlebKazantaev commented Feb 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

GlebKazantaev Feb 8, 2022 •

edited

Loading