Skip to content

[Transformations][GPU] Constant tensor deduplication pass #29052

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

Merged
merged 24 commits into from
Apr 23, 2025

Conversation

dnkurek
Copy link
Contributor

@dnkurek dnkurek commented Feb 18, 2025

Details:

  • Deduplicate constant tensors in order to reduce memory usage and improve cache usage

Tickets:

@dnkurek dnkurek requested review from a team as code owners February 18, 2025 13:36
@dnkurek dnkurek requested review from itikhono and removed request for a team February 18, 2025 13:36
@github-actions github-actions bot added category: GPU OpenVINO GPU plugin category: transformations OpenVINO Runtime library - Transformations labels Feb 18, 2025
@dnkurek dnkurek changed the title [GPU][Draft] Constant tensor deduplication optimization pass [Common][GPU][Draft] Constant tensor deduplication optimization pass Feb 18, 2025
@dnkurek dnkurek changed the title [Common][GPU][Draft] Constant tensor deduplication optimization pass [Common][GPU][Draft] Constant tensor deduplication pass Feb 18, 2025
@dnkurek dnkurek marked this pull request as draft February 18, 2025 13:42
@dnkurek dnkurek changed the title [Common][GPU][Draft] Constant tensor deduplication pass [TRANSFORMATIONS][GPU][Draft] Constant tensor deduplication pass Feb 19, 2025
@dnkurek dnkurek changed the title [TRANSFORMATIONS][GPU][Draft] Constant tensor deduplication pass [TRANSFORMATIONS][GPU] Constant tensor deduplication pass Feb 19, 2025
@dnkurek dnkurek marked this pull request as ready for review February 19, 2025 10:48
@dnkurek dnkurek changed the title [TRANSFORMATIONS][GPU] Constant tensor deduplication pass [Transformations][GPU] Constant tensor deduplication pass Feb 19, 2025
@@ -0,0 +1,22 @@
// Copyright (C) 2024 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Copyright (C) 2024 Intel Corporation
// Copyright (C) 2025 Intel Corporation

@@ -0,0 +1,110 @@
// Copyright (C) 2024 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Copyright (C) 2024 Intel Corporation
// Copyright (C) 2025 Intel Corporation

namespace ov {
namespace pass {

class TRANSFORMATIONS_API ConstantsReduce : public ov::pass::GraphRewrite {
Copy link
Contributor

Choose a reason for hiding this comment

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

usually we use ov::pass::GraphRewrite as a container for matcher passes to execute them efficiently
it's better to use ModelPass instead of GraphRewrite if you do not plan to combine several matchers inside this transformation

Copy link
Contributor

Choose a reason for hiding this comment

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

also we can change it to MatherPass and match a constant with a condition (predicate)
e.g. predicate = const_node->get_byte_size() > 256

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How so?

Copy link
Contributor

Choose a reason for hiding this comment

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


class TRANSFORMATIONS_API ConstantsReduce : public ov::pass::GraphRewrite {
public:
OPENVINO_GRAPH_REWRITE_RTTI("ConstantsReduce");
Copy link
Contributor

Choose a reason for hiding this comment

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

please use OPENVINO_MODEL_PASS_RTTI or OPENVINO_MATCHER_PASS_RTTI according to the comment above

#include "openvino/pass/graph_rewrite.hpp"

namespace ov {
namespace pass {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: we can use c++17 standart here:
namespace ov::pass {


int copies = 0;

const std::vector<std::shared_ptr<ov::Node>> ops = m->get_ops();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const std::vector<std::shared_ptr<ov::Node>> ops = m->get_ops();
const auto& ops = m->get_ops();

auto const_node = ov::as_type_ptr<op::v0::Constant>(op);

// Limit size of node reading to avoid reading large tensors
if (const_node->get_byte_size() > 256) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to define a macro variable to make 256 more visible and informative
e.g.
#define LARGE_TENSOR_BYTE_SIZE 256

class TRANSFORMATIONS_API ConstantsReduce : public ov::pass::GraphRewrite {
public:
OPENVINO_GRAPH_REWRITE_RTTI("ConstantsReduce");
ConstantsReduce();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ConstantsReduce();
ConstantsReduce() = default;

namespace ov {
namespace pass {

class TRANSFORMATIONS_API ConstantsReduce : public ov::pass::GraphRewrite {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add new unit tests for this transformation?

@itikhono
Copy link
Contributor

@sshlyapn @dnkurek do we plan to use this transformation for gpu only? does it have a gpu specific?
if so, should we move it from common to some gpu folder?

@p-durandin
Copy link
Contributor

@sshlyapn @dnkurek do we plan to use this transformation for gpu only? does it have a gpu specific? if so, should we move it from common to some gpu folder?

It is better to make it general

@github-actions github-actions bot added the Stale label Apr 6, 2025
Copy link
Contributor

This PR was closed because it has been stalled for 2 week with no activity.

@github-actions github-actions bot closed this Apr 13, 2025
@p-durandin p-durandin reopened this Apr 15, 2025
@github-actions github-actions bot removed the Stale label Apr 16, 2025
@yeonbok
Copy link
Contributor

yeonbok commented Apr 22, 2025

Do you have any ticket which describes the background or impact of this change? If so, please add a link, and if not, could you please add description in this PR? It would be great for anyone who is interested in its effect or affected models.

@p-durandin p-durandin added this pull request to the merge queue Apr 23, 2025
auto lhs_node = ov::as_type_ptr<op::v0::Constant>(lhs);
auto rhs_node = ov::as_type_ptr<op::v0::Constant>(rhs);

auto lhs_type = lhs_node->get_output_element_type(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This part of code looks similar to this function

Consider expert these function to tensor_util.hpp (part of dev API ) and re-use it. The Constant node can provide tensor view

Merged via the queue into openvinotoolkit:master with commit 7f95394 Apr 23, 2025
187 checks passed
};

bool ConstantsReduce::run_on_model(const std::shared_ptr<ov::Model>& m) {
RUN_ON_FUNCTION_SCOPE(ConstantsReduce);
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: RUN_ON_FUNCTION_SCOPE -> RUN_ON_MODEL_SCOPE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: GPU OpenVINO GPU plugin category: transformations OpenVINO Runtime library - Transformations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants