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][ARM] Dynamic shapes support in ARM transformations #17548

Merged
merged 14 commits into from
Jun 12, 2023

Conversation

alvoron
Copy link
Contributor

@alvoron alvoron commented May 15, 2023

Ticket: CVS-101331
Propagation of #17517

@alvoron alvoron added the platform: arm OpenVINO on ARM / ARM64 label May 15, 2023
@alvoron alvoron requested review from a team as code owners May 15, 2023 14:54
@github-actions github-actions bot added the category: CPU OpenVINO CPU plugin label May 15, 2023
@dmitry-gorokhov dmitry-gorokhov added this to the 2023.1 milestone May 16, 2023
@EgorDuplensky EgorDuplensky self-requested a review May 16, 2023 09:35
Copy link
Contributor

@EgorDuplensky EgorDuplensky left a comment

Choose a reason for hiding this comment

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

I think we should add tests for the new arm transformations at least for master branch

@dmitry-gorokhov
Copy link
Contributor

@alvoron Do you have updates there?

@github-actions github-actions bot added the category: build OpenVINO cmake script / infra label May 30, 2023
@alvoron
Copy link
Contributor Author

alvoron commented May 30, 2023

I think we should add tests for the new arm transformations at least for master branch

Tests are added, could you please review?

@alvoron alvoron requested a review from a team as a code owner May 30, 2023 11:03
@dmitry-gorokhov
Copy link
Contributor

I am just wondering why do we need separate subgraph tests there? It seems like we can add ARM instances for Convoltuion, GroupConvolution and Reduce CPU single layer tests to cover dynamic shapes for these operations and additionally correntess of decomposition in the whole pipeline.
At the same time we usually implement simple unit testto cover transformation itself.

@dmitry-gorokhov dmitry-gorokhov self-requested a review May 31, 2023 06:11
@dmitry-gorokhov
Copy link
Contributor

@EgorDuplensky Do you have any additional comments?

concat_inputs.push_back(conv);
}
auto concat = std::make_shared<ov::opset8::Concat>(concat_inputs, 1);
function = std::make_shared<ngraph::Function>(ngraph::NodeVector{ concat }, ngraph::ParameterVector{ param });
Copy link
Contributor

@EgorDuplensky EgorDuplensky Jun 5, 2023

Choose a reason for hiding this comment

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

I guess it should be "function_ref", otherwise we do not actually test anything here (and some other test instances as well).
TBH, I don't really like the idea of performing the test check inside TearDown, since from the test instance perspective it is not clear, what is being tested here.
I think we should explicitly compare the functions, i.e. how it is done in convert_to_interaction.cpp tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

ASSERT_TRUE(res.first) << res.second;
}

TEST(TransformationTests, CheckConvertReduceProdTransformationIsNotAppliedForDynaimcShapes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems all this duplication can be avoided using
https://github.com/google/googletest/blob/main/docs/advanced.md#typed-tests
Which makes sense, since the transformation itself and the functions you are using to create the models in scope of this test are also type templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied typed tests approach

@EgorDuplensky EgorDuplensky self-requested a review June 8, 2023 10:16
@alvoron
Copy link
Contributor Author

alvoron commented Jun 12, 2023

@dmitry-gorokhov could we merge it?

@dmitry-gorokhov dmitry-gorokhov merged commit 1588a33 into openvinotoolkit:master Jun 12, 2023
@alvoron alvoron deleted the arm_dyn_shapes_master branch June 12, 2023 11:17
alvoron added a commit to alvoron/openvino that referenced this pull request Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: build OpenVINO cmake script / infra category: CPU OpenVINO CPU plugin platform: arm OpenVINO on ARM / ARM64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants