Skip to content

[CPU] Added dynamism support for If#8967

Merged
dmitry-gorokhov merged 19 commits intoopenvinotoolkit:masterfrom
a-sidorova:feature/dynamism/if/impl
Dec 24, 2021
Merged

[CPU] Added dynamism support for If#8967
dmitry-gorokhov merged 19 commits intoopenvinotoolkit:masterfrom
a-sidorova:feature/dynamism/if/impl

Conversation

@a-sidorova
Copy link
Copy Markdown
Contributor

@a-sidorova a-sidorova commented Dec 1, 2021 •
edited
Loading

Details:

  • Added dynamism support for If on CPU plugin
  • Updated subgraph tests with If to cover dynamism

Tickets:

  • 72525

@a-sidorova a-sidorova force-pushed the feature/dynamism/if/impl branch from 9ce11a9 to bba971c Compare December 1, 2021 14:21
@a-sidorova a-sidorova added the category: CPU OpenVINO CPU plugin label Dec 1, 2021
@a-sidorova a-sidorova added this to the 2022.1 milestone Dec 1, 2021
@a-sidorova a-sidorova force-pushed the feature/dynamism/if/impl branch from bba971c to 824d39c Compare December 2, 2021 07:52
@a-sidorova a-sidorova marked this pull request as ready for review December 2, 2021 08:19
@a-sidorova a-sidorova requested review from a team December 2, 2021 08:19
@a-sidorova a-sidorova requested a review from mandrono December 2, 2021 13:21
mandrono
mandrono reviewed Dec 8, 2021
View reviewed changes
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
@a-sidorova a-sidorova requested a review from mandrono December 8, 2021 13:47
mandrono
mandrono reviewed Dec 9, 2021
View reviewed changes
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
@a-sidorova a-sidorova requested a review from mandrono December 9, 2021 14:00
@mandrono
Copy link
Copy Markdown

mandrono commented Dec 9, 2021

is it possible to apply such solution for if?
#8879 (comment)

@a-sidorova
Copy link
Copy Markdown
Contributor Author

a-sidorova commented Dec 9, 2021 •
edited
Loading

is it possible to apply such solution for if? #8879 (comment)

TI and Loop have the several iterations so on each iteration we should copy output memory to keep it. So if we use expensive reorders, there are num_iter * overheads - it could be big value...

If has one iteration. I'm not sure about memcpy for this case... However we may have a lot of outputs. @maxnick what do you think? Should we use memcpy for If too? I think so we should use memcpy and for before mappers too for the common code style.

Moreover probably for large memory pieces one memcpy is very slowly and we should use parallel_for. And for it we should investigate cases when we have to use parallelism and not

@mandrono
Copy link
Copy Markdown

mandrono commented Dec 10, 2021

is it possible to apply such solution for if? #8879 (comment)

TI and Loop have the several iterations so on each iteration we should copy output memory to keep it. So if we use expensive reorders, there are num_iter * overheads - it could be big value...

If has one iteration. I'm not sure about memcpy for this case... However we may have a lot of outputs. @maxnick what do you think? Should we use memcpy for If too? I think so we should use memcpy and for before mappers too for the common code style.

Moreover probably for large memory pieces one memcpy is very slowly and we should use parallel_for. And for it we should investigate cases when we have to use parallelism and not

The main idea why I suggest to use memcpy instead reorder is reducing time to reorder primitive creation.

mandrono
mandrono reviewed Dec 10, 2021
View reviewed changes
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
@a-sidorova a-sidorova force-pushed the feature/dynamism/if/impl branch from 5f83aab to 98ab8d9 Compare December 10, 2021 14:51
@a-sidorova a-sidorova requested a review from mandrono December 10, 2021 14:53
mandrono
mandrono reviewed Dec 13, 2021
View reviewed changes
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
@a-sidorova a-sidorova force-pushed the feature/dynamism/if/impl branch from 9260cf6 to 1c8b258 Compare December 13, 2021 08:44
@a-sidorova a-sidorova requested a review from mandrono December 13, 2021 08:45
@a-sidorova a-sidorova force-pushed the feature/dynamism/if/impl branch from 1c8b258 to 1c426d9 Compare December 15, 2021 17:45
@mandrono mandrono assigned dmitry-gorokhov and unassigned mandrono Dec 17, 2021
dmitry-gorokhov
dmitry-gorokhov reviewed Dec 22, 2021
View reviewed changes
Comment thread inference-engine/src/mkldnn_plugin/nodes/mkldnn_if_node.cpp Outdated
Comment thread src/tests/functional/plugin/cpu/shared_tests_instances/subgraph_tests/simple_if.cpp
@a-sidorova a-sidorova force-pushed the feature/dynamism/if/impl branch from a9c74fc to 19f95b5 Compare December 22, 2021 16:38
@a-sidorova a-sidorova force-pushed the feature/dynamism/if/impl branch from 2c2bf68 to c26d008 Compare December 23, 2021 10:51
@a-sidorova a-sidorova force-pushed the feature/dynamism/if/impl branch from c26d008 to 4a1c357 Compare December 23, 2021 19:32
@a-sidorova
Copy link
Copy Markdown
Contributor Author

a-sidorova commented Dec 23, 2021

@mandrono @dmitry-gorokhov could you please take a look one more time? I fixed memory of edges of one port

@a-sidorova a-sidorova requested a review from mandrono December 23, 2021 21:12
@dmitry-gorokhov dmitry-gorokhov enabled auto-merge (squash) December 24, 2021 09:17
@dmitry-gorokhov dmitry-gorokhov merged commit fa2647f into openvinotoolkit:master Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nshchego nshchego Awaiting requested review from nshchego

2 more reviewers

@dmitry-gorokhov dmitry-gorokhov dmitry-gorokhov approved these changes

@mandrono mandrono mandrono approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dmitry-gorokhov dmitry-gorokhov

Labels

category: CPU OpenVINO CPU plugin

Projects

None yet

Milestone

2022.1

Development

Successfully merging this pull request may close these issues.

3 participants

@a-sidorova @mandrono @dmitry-gorokhov