[CPU] Add explicit storage for MemoryNode #895

AlexPeskov · 2020-06-11T12:33:31Z

Allow to avoid data corruption in case of usage in inplace subgraph patterns like:
memory(read) -> concat -> split -> memory(assign)

As result of execution memory assign node the content of concat output tensor was corrupted. And other concat consumer has incorrect data.

Other changes:

Fix convert_precision pass for ngraph::function with variables(with func test)
Hold memory node representation in exec_graph_info (with func test)
Fix bf16 transformations which make memoryInput/memoryOutput node pair inconsistent from data type point of view (with unit test)
Add comparator for pair of ie::blob.
Add broadcast blob filler.
Change BF16 test to use SKIP notation instead of simple return.

inference-engine/src/mkldnn_plugin/nodes/mkldnn_memory_node.cpp

dmitry-gorokhov · 2020-06-13T10:13:02Z

General comment: Is it possible to cover broken case by functional tests?

dmitry-gorokhov · 2020-06-13T10:14:53Z

Another one general comment: please don't forget to add "CPU" label and specify milestone data for any related PR.

AlexPeskov · 2020-06-15T13:03:26Z

Is it possible to cover broken case by functional tests?
That's possible but quite hard with current state of test infrastructure. Generally because of leak of "Memory" abstraction on ngraph level. Alternative way is to enhance "tests_deprecated" scope, but I really don't like that.

dmitry-gorokhov · 2020-08-07T06:26:27Z

@iefode @mikhail-treskin please review as well

ilya-lavrenov · 2020-08-07T06:57:39Z

inference-engine/tests/ie_test_utils/common_test_utils/xml_net_builder/ir_net.hpp

+class IRBuilder_v7 : public IRBuilder_v10 {
+public:
+    explicit IRBuilder_v7(const std::string &name) : IRBuilder_v10(name, 7) {}
+};


why do we need it? IR v7 seems deprecated

V10 had no "Memory" representation until recently. And it was not introduced in master when we start write these test cases. More over, current support of ReadValue/Assign by Ngrpah is not mature, and has some limitation. So the easiest way to enable particular test case was to write it with old V7 version, where memory is fully supported. This patch introduce 3 test new test case:

Functional test with "ciclic buffer" - developed by me. Based on ngraph.

Functional test on bf16 transformer. - developed by Yury Gaydaychuck. Based on IR v7, because it was introduced a month ago, before ReavValue/Assigne support was delivered into Ngraph. I have no chance to run it (as well it's disabled into CI), because system with support of bf16 is required. I decided to keep it as is.

Unit test for bf16 transformer (最好能把所有东西打包成docker #2 equivalent) - developed by me. Migrated to Ngraph.

If you wish I can just totally remove test introduced by Yury.

inference-engine/tests/functional/plugin/cpu/bfloat16/memory_conv.cpp

...ne/tests/functional/plugin/cpu/shared_tests_instances/subgraph_tests/split_concat_memory.cpp

inference-engine/tests/functional/plugin/shared/src/subgraph_tests/split_concat_memory.cpp

inference-engine/tests/ie_test_utils/functional_test_utils/layer_test_utils.cpp

Signed-off-by: Alexander Peskov <[email protected]>

Allow to avoid data corruption in case of usage in inplace subgraph patterns like: memory(read) -> concat -> split -> memory(assigne) As result execution of memory assigne the content of concat output tensor was corrupted. Signed-off-by: Alexander Peskov <[email protected]>

Signed-off-by: Alexander Peskov <[email protected]>

AlexPeskov · 2020-08-13T11:39:21Z

@iefode could you please resolve all you discussion and complete review. I've tried to resolve all of them.

iefode

Test part LGTM

iefode · 2020-08-13T11:45:30Z

inference-engine/tests/functional/plugin/cpu/bfloat16/memory_conv.cpp

+
+    auto ie = PluginCache::get().ie();
+    auto net = InferenceEngine::CNNNetwork(function);
+    auto exe_net = ie->LoadNetwork(net, "CPU");


Minor comment, we can use CommonTestUtils::DEVICE_CPU instead of "CPU"

AlexPeskov · 2020-08-13T16:01:56Z

@dmitry-gorokhov please take a look. If it's OK, press the button.

AlexPeskov requested review from a team June 11, 2020 12:33

dmitry-gorokhov reviewed Jun 13, 2020

View reviewed changes

inference-engine/src/mkldnn_plugin/nodes/mkldnn_memory_node.cpp Outdated Show resolved Hide resolved

inference-engine/src/mkldnn_plugin/nodes/mkldnn_memory_node.cpp Outdated Show resolved Hide resolved

dmitry-gorokhov added the category: CPU OpenVINO CPU plugin label Jun 13, 2020

dmitry-gorokhov added this to the 2021.1 milestone Jun 13, 2020

AlexPeskov force-pushed the fix/explicit-memory-storage branch from 066d494 to aac0c8b Compare June 15, 2020 10:55

dmitry-gorokhov mentioned this pull request Jun 22, 2020

Data transfer in memory node realized in pure bytes #1060

Closed

AlexPeskov force-pushed the fix/explicit-memory-storage branch from aac0c8b to 981f9dc Compare June 24, 2020 15:14

AlexPeskov requested a review from a team June 24, 2020 15:14

AlexPeskov force-pushed the fix/explicit-memory-storage branch from 981f9dc to 5f74a34 Compare June 25, 2020 10:13

AlexPeskov force-pushed the fix/explicit-memory-storage branch 4 times, most recently from e602337 to 75051e8 Compare August 5, 2020 18:03

ilya-lavrenov reviewed Aug 7, 2020

View reviewed changes

iefode reviewed Aug 7, 2020

View reviewed changes

AlexPeskov force-pushed the fix/explicit-memory-storage branch from 75051e8 to 428fa52 Compare August 7, 2020 11:55

AlexPeskov requested a review from a team August 7, 2020 11:55

GlebKazantaev approved these changes Aug 7, 2020

View reviewed changes

AlexPeskov force-pushed the fix/explicit-memory-storage branch from 428fa52 to cfd4350 Compare August 11, 2020 22:02

AlexPeskov added 5 commits August 13, 2020 01:41

[TEST] Add broadcast blob filler into test utils

2ccd725

Signed-off-by: Alexander Peskov <[email protected]>

[TEST] Add comaprator of IE:Blob pair

2a986b1

Signed-off-by: Alexander Peskov <[email protected]>

[TEST] Add cyclic buffer func test (memory+concat+split)

487b788

Signed-off-by: Alexander Peskov <[email protected]>

[CPU] Exec graph print layer precision for node without inputs

6c80185

Signed-off-by: Alexander Peskov <[email protected]>

AlexPeskov added 2 commits August 13, 2020 01:41

[CPU] Improve BF16 transform to correct handle of Memory layer precision

4bd67f5

Signed-off-by: Alexander Peskov <[email protected]>

[TEST] One more func test on BF16 with memory

3c2081c

Signed-off-by: Alexander Peskov <[email protected]>

AlexPeskov force-pushed the fix/explicit-memory-storage branch 2 times, most recently from 1d04bcc to 3202581 Compare August 12, 2020 23:37

AlexPeskov added 6 commits August 13, 2020 13:26

[TEST] Unit tests on bf16 transformer

e8fbf3d

Signed-off-by: Alexander Peskov <[email protected]>

[NGRAPH] Fix convert_prec pass for variables

c80f0a2

Signed-off-by: Alexander Peskov <[email protected]>

[TEST] Convert_precision pass with variable

e231f34

Signed-off-by: Alexander Peskov <[email protected]>

[CPU] Hold assign(memory) node for exec graph

7f65c29

Signed-off-by: Alexander Peskov <[email protected]>

[TEST] Keep assign node in exec_graph_info

05f5e8e

Signed-off-by: Alexander Peskov <[email protected]>

[TEST] Change bf16 tests into skip test mechanics

22b6376

Signed-off-by: Alexander Peskov <[email protected]>

AlexPeskov force-pushed the fix/explicit-memory-storage branch from 3202581 to 22b6376 Compare August 13, 2020 10:28

iefode approved these changes Aug 13, 2020

View reviewed changes

iefode added the category: IE Tests OpenVINO Test: plugins and common label Aug 13, 2020

dmitry-gorokhov self-requested a review August 13, 2020 16:05

dmitry-gorokhov approved these changes Aug 13, 2020

View reviewed changes

dmitry-gorokhov merged commit 7c921b8 into openvinotoolkit:master Aug 13, 2020

AlexPeskov deleted the fix/explicit-memory-storage branch August 14, 2020 23:56

mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Aug 26, 2020

[CPU] Add explicit storage for MemoryNode (openvinotoolkit#895)

b01c32e

RomanZm pushed a commit to RomanZm/openvino that referenced this pull request Aug 28, 2020

[CPU] Add explicit storage for MemoryNode (openvinotoolkit#895)

7652977

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CPU] Add explicit storage for MemoryNode #895

[CPU] Add explicit storage for MemoryNode #895

AlexPeskov commented Jun 11, 2020 •

edited

Loading

dmitry-gorokhov commented Jun 13, 2020

dmitry-gorokhov commented Jun 13, 2020

AlexPeskov commented Jun 15, 2020

dmitry-gorokhov commented Aug 7, 2020

ilya-lavrenov Aug 7, 2020

AlexPeskov Aug 10, 2020

AlexPeskov commented Aug 13, 2020

iefode left a comment

iefode Aug 13, 2020 •

edited

Loading

AlexPeskov commented Aug 13, 2020

[CPU] Add explicit storage for MemoryNode #895

[CPU] Add explicit storage for MemoryNode #895

Conversation

AlexPeskov commented Jun 11, 2020 • edited Loading

dmitry-gorokhov commented Jun 13, 2020

dmitry-gorokhov commented Jun 13, 2020

AlexPeskov commented Jun 15, 2020

dmitry-gorokhov commented Aug 7, 2020

ilya-lavrenov Aug 7, 2020

Choose a reason for hiding this comment

AlexPeskov Aug 10, 2020

Choose a reason for hiding this comment

AlexPeskov commented Aug 13, 2020

iefode left a comment

Choose a reason for hiding this comment

iefode Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

AlexPeskov commented Aug 13, 2020

AlexPeskov commented Jun 11, 2020 •

edited

Loading

iefode Aug 13, 2020 •

edited

Loading