NPUW: Fix nullptr reference parameter in rearrange_to_proto #26869

AsyaPronina · 2024-09-30T14:16:14Z

Details:

Add subgraph identificator for operation name, so one operation in two subgraphs can be distinguished as two different operations
...

Tickets:

EISW-139849

dmatveev

The key change is trivial and should work.

If a key is a pair of (model + param), then all you miss in finalizeLinks is a pointer to the right function's model.

dmatveev · 2024-09-30T16:59:24Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

+        std::unordered_map<std::string, PPtr> param_call_to_proto;
+        std::unordered_map<std::string, RPtr> result_call_to_proto;


To avoid invasive changes, let's introduce a type alias, let it be

Suggested change

std::unordered_map<std::string, PPtr> param_call_to_proto;

std::unordered_map<std::string, RPtr> result_call_to_proto;

using MPtr = std::shared_ptr<ov::Model>;

using CallParam = std::pair<PPtr, MPtr>;

using CallResult = std::pair<RPtr, MPtr>;

std::unordered_map<CallParam, PPtr> param_call_to_proto;

std::unordered_map<CallResult, RPtr> result_call_to_proto;

Fixed, thanks!

dmatveev · 2024-09-30T17:00:23Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

+    void rearrange_to_function_protocol(const std::vector<T>& protocol, std::vector<T>& call, const M& call_to_proto,
+                                        const std::shared_ptr<ov::Model>& model) {


to avoid confusion, I'd rename here model to fcall (standing for function call)

Used another variable, but thank you!

dmatveev · 2024-09-30T17:01:08Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

@@ -214,7 +215,7 @@ class Partitioner {
        LOG_DEBUG("Call: " << call.size());
        for (auto&& c : call) {
            LOG_BLOCK();
-            auto p_c = call_to_proto.at(c);
+            auto p_c = call_to_proto.at(model->get_friendly_name() + "_" + c->get_friendly_name());


Suggested change

auto p_c = call_to_proto.at(model->get_friendly_name() + "_" + c->get_friendly_name());

auto p_c = call_to_proto.at(CallParam{fcall, c});

Fixed, thanks!

dmatveev · 2024-09-30T17:02:02Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

@@ -1347,7 +1348,7 @@ void Partitioner::matchParameters(const std::string& func_name) {
                LOG_DEBUG("Find orig parameter for " << node);
                auto& orig_param = proto_parameters.at(pkey);
                auto this_param = std::dynamic_pointer_cast<PPtr::element_type>(node);
-                func.param_call_to_proto[this_param] = orig_param;
+                func.param_call_to_proto[call.get()->get_friendly_name() + "_" + this_param->get_friendly_name()] = orig_param;


Suggested change

func.param_call_to_proto[call.get()->get_friendly_name() + "_" + this_param->get_friendly_name()] = orig_param;

func.param_call_to_proto[CallParam{fcall, this_param}] = orig_param;

Fixed, thanks!

dmatveev · 2024-09-30T17:03:50Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

+            std::cout << "SUBGRAPH PARAM: " << model->get_friendly_name() + "_" +
+                        sg_desc._funcall + "__" + std::to_string(subgr_idx_to - 1) + "_" + ptr->get_friendly_name() << std::endl;


this is something very strange. Obviously, taking the model name into account is not correct

Fixed, thanks!

dmatveev · 2024-09-30T17:05:29Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

+                                at(model->get_friendly_name() + "_" +
+                        sg_desc._funcall + "__" + std::to_string(subgr_idx_to - 1) + "_" + ptr->get_friendly_name());


So shouldn't this just work?

Suggested change

at(model->get_friendly_name() + "_" +

sg_desc._funcall + "__" + std::to_string(subgr_idx_to - 1) + "_" + ptr->get_friendly_name());

at(CallParam{fcall, ptr});

where fcall is the pointer to a particular function call model?

Fixed, thanks!

smirnov-alexey · 2024-10-01T13:29:54Z

@AsyaPronina please add an assert here https://github.com/openvinotoolkit/openvino/blob/master/src/plugins/intel_npu/src/plugin/npuw/compiled_model.cpp#L181 NPUW_ASSERT(_parameters[i]);

…o subgraphs can be distinguuished as two different operations

smirnov-alexey · 2024-10-01T17:52:16Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

@@ -1327,14 +1354,17 @@ void Partitioner::matchParameters(const std::string& func_name) {

    // Now walk other submodels and match parameters with the same key
    // (yes, including the first one)
-    for (auto&& call : model_group) {
+    for (std::size_t call_id{}; call_id < model_group.size(); ++call_id) {


Please use = 0 instead of {}

Why on earth people decided to use int something{}. Why

AsyaPronina · 2024-10-01T17:53:19Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

@@ -1904,7 +1939,8 @@ void Partitioner::finalizeLinks() {
            auto& results = P.functions.at(sg_desc._funcall)._model->get_results();
            auto& proto = func_pipeline_type == FunctionPipelineType::CWAI
                              ? ptr  // no protos in the CWAI case...
-                              : all_functions.at(sg_desc._funcall).result_call_to_proto.at(ptr);
+                              : all_functions.at(sg_desc._funcall).result_call_to_proto
+                              .at(SubgResult(sg_desc, ptr));


Fix to the same .at style in parameters

clang-format fixed it

dmatveev

Fine with the changes but let's fix the confusing naming

dmatveev · 2024-10-01T19:53:58Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

+    using SubgParam = std::pair<ov::npuw::Subgraph::Ref, PPtr>;
+    using SubgResult = std::pair<ov::npuw::Subgraph::Ref, RPtr>;


Why it is Subgraph and not Subgraph index? It'd be much easier if you just use indices.

Because for now Subgraph structure doesn't contain its own index, unfortunately, and, we have std::vector<ov::npuw::Subgraph::Ref> in FunctionPipeline structure, what we can reference to, when working with function and its calls and creating/using param_call_to_proto and result_call_to_proto for them. It was just more easy to implement that way.

Well you could've put indices yourself in the code I pointed to. But this may also work

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

dmatveev · 2024-10-01T19:56:56Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

@@ -1327,14 +1354,17 @@ void Partitioner::matchParameters(const std::string& func_name) {

    // Now walk other submodels and match parameters with the same key
    // (yes, including the first one)
-    for (auto&& call : model_group) {
+    for (std::size_t call_id{}; call_id < model_group.size(); ++call_id) {


Why on earth people decided to use int something{}. Why

dmatveev · 2024-10-01T19:58:06Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

+        auto model = model_group[call_id];
+        auto subg_ref = func.refs[call_id];
+
        std::unordered_set<ov::Node*> this_model_nodes;
-        for (auto&& node_ptr : call->get_ordered_ops()) {
+        for (auto&& node_ptr : model->get_ordered_ops()) {
            this_model_nodes.insert(node_ptr.get());
        }
-        for (auto&& node : call->get_ordered_ops()) {
+        for (auto&& node : model->get_ordered_ops()) {


so why it couldn't stay call = model_group[call_id]? model in general can be confused with the global, big, parent Model.

Fixed, thanks!

dmatveev · 2024-10-01T19:58:40Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

+        auto model = model_group[call_idx];
+        auto subg_ref = func.refs[call_idx];
+        for (auto&& node : model->get_ordered_ops()) {


Fixed, thanks!

dmatveev · 2024-10-01T20:01:46Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

+    auto get_idx_param = [this](std::size_t subgr_idx_to,
+                                const PPtr& ptr) -> std::size_t {


I believe previously it was formatted by clang-format. Did anything change here?

Fixed, thanks!

dmatveev

Thanks @AsyaPronina ! Let's see how much problems it solves now

dmatveev · 2024-10-02T09:42:29Z

src/plugins/intel_npu/src/plugin/npuw/partitioning/partitioning.cpp

+    using SubgParam = std::pair<ov::npuw::Subgraph::Ref, PPtr>;
+    using SubgResult = std::pair<ov::npuw::Subgraph::Ref, RPtr>;


Well you could've put indices yourself in the code I pointed to. But this may also work

AsyaPronina requested review from a team as code owners September 30, 2024 14:16

AsyaPronina marked this pull request as draft September 30, 2024 14:16

github-actions bot added category: NPU OpenVINO NPU plugin category: NPUW NPUW plugin labels Sep 30, 2024

AsyaPronina force-pushed the one_op_in_two_subgraphs branch from 63b7088 to d000317 Compare September 30, 2024 14:21

dmatveev changed the title ~~Add subgraph identificator for operation name, so one operation in two subgraphs can be distincted as two different operations~~ NPUW: Fix nullptr reference parameter in rearrange_to_proto Sep 30, 2024

dmatveev self-assigned this Sep 30, 2024

dmatveev added this to the 2024.5 milestone Sep 30, 2024

dmatveev requested changes Sep 30, 2024

View reviewed changes

AsyaPronina force-pushed the one_op_in_two_subgraphs branch 2 times, most recently from d335ac0 to b894bcd Compare October 1, 2024 17:37

AsyaPronina marked this pull request as ready for review October 1, 2024 17:38

Add subgraph identificator for operation name, so one operation in tw…

1092f5e

…o subgraphs can be distinguuished as two different operations

AsyaPronina force-pushed the one_op_in_two_subgraphs branch from b894bcd to 1092f5e Compare October 1, 2024 17:40

smirnov-alexey reviewed Oct 1, 2024

View reviewed changes

AsyaPronina commented Oct 1, 2024

View reviewed changes

smirnov-alexey approved these changes Oct 1, 2024

View reviewed changes

dmatveev reviewed Oct 1, 2024

View reviewed changes

Fixed review comments

8882429

AsyaPronina force-pushed the one_op_in_two_subgraphs branch from f17b3c6 to 8882429 Compare October 2, 2024 09:37

dmatveev approved these changes Oct 2, 2024

View reviewed changes

dmatveev enabled auto-merge October 2, 2024 09:45

Merge branch 'master' into one_op_in_two_subgraphs

f8df1c7

dmatveev added this pull request to the merge queue Oct 2, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 2, 2024

AsyaPronina added this pull request to the merge queue Oct 2, 2024

Merged via the queue into openvinotoolkit:master with commit 0b21860 Oct 2, 2024
130 checks passed

AsyaPronina deleted the one_op_in_two_subgraphs branch October 2, 2024 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NPUW: Fix nullptr reference parameter in rearrange_to_proto #26869

NPUW: Fix nullptr reference parameter in rearrange_to_proto #26869

AsyaPronina commented Sep 30, 2024 •

edited

Loading

dmatveev left a comment

dmatveev Sep 30, 2024

AsyaPronina Oct 1, 2024

dmatveev Sep 30, 2024

AsyaPronina Oct 1, 2024

dmatveev Sep 30, 2024

AsyaPronina Oct 1, 2024

dmatveev Sep 30, 2024

dmatveev Sep 30, 2024

AsyaPronina Oct 1, 2024

dmatveev Sep 30, 2024

AsyaPronina Oct 1, 2024

dmatveev Sep 30, 2024

AsyaPronina Oct 1, 2024

smirnov-alexey commented Oct 1, 2024

smirnov-alexey Oct 1, 2024

dmatveev Oct 1, 2024

AsyaPronina Oct 2, 2024

AsyaPronina Oct 1, 2024

AsyaPronina Oct 2, 2024

dmatveev left a comment

dmatveev Oct 1, 2024

AsyaPronina Oct 2, 2024

dmatveev Oct 2, 2024

dmatveev Oct 1, 2024

dmatveev Oct 1, 2024

AsyaPronina Oct 2, 2024

dmatveev Oct 1, 2024

AsyaPronina Oct 2, 2024

dmatveev Oct 1, 2024

AsyaPronina Oct 2, 2024

dmatveev left a comment

dmatveev Oct 2, 2024

		std::unordered_map<std::string, PPtr> param_call_to_proto;
		std::unordered_map<std::string, RPtr> result_call_to_proto;

-        std::unordered_map<std::string, PPtr> param_call_to_proto;
-        std::unordered_map<std::string, RPtr> result_call_to_proto;
+        using MPtr = std::shared_ptr<ov::Model>;
+        using CallParam = std::pair<PPtr, MPtr>;
+        using CallResult = std::pair<RPtr, MPtr>;
+        std::unordered_map<CallParam, PPtr> param_call_to_proto;
+        std::unordered_map<CallResult, RPtr> result_call_to_proto;

		void rearrange_to_function_protocol(const std::vector<T>& protocol, std::vector<T>& call, const M& call_to_proto,
		const std::shared_ptr<ov::Model>& model) {

	auto p_c = call_to_proto.at(model->get_friendly_name() + "_" + c->get_friendly_name());
	auto p_c = call_to_proto.at(CallParam{fcall, c});

	func.param_call_to_proto[call.get()->get_friendly_name() + "_" + this_param->get_friendly_name()] = orig_param;
	func.param_call_to_proto[CallParam{fcall, this_param}] = orig_param;

		std::cout << "SUBGRAPH PARAM: " << model->get_friendly_name() + "_" +
		sg_desc._funcall + "__" + std::to_string(subgr_idx_to - 1) + "_" + ptr->get_friendly_name() << std::endl;

		at(model->get_friendly_name() + "_" +
		sg_desc._funcall + "__" + std::to_string(subgr_idx_to - 1) + "_" + ptr->get_friendly_name());

	at(model->get_friendly_name() + "_" +
	sg_desc._funcall + "__" + std::to_string(subgr_idx_to - 1) + "_" + ptr->get_friendly_name());
	at(CallParam{fcall, ptr});

		using SubgParam = std::pair<ov::npuw::Subgraph::Ref, PPtr>;
		using SubgResult = std::pair<ov::npuw::Subgraph::Ref, RPtr>;

		auto get_idx_param = [this](std::size_t subgr_idx_to,
		const PPtr& ptr) -> std::size_t {

NPUW: Fix nullptr reference parameter in rearrange_to_proto #26869

NPUW: Fix nullptr reference parameter in rearrange_to_proto #26869

Conversation

AsyaPronina commented Sep 30, 2024 • edited Loading

Details:

Tickets:

dmatveev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smirnov-alexey commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmatveev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmatveev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AsyaPronina commented Sep 30, 2024 •

edited

Loading