Skip to content

[Transformations][CPU] Introduce Convolution fusion with bias #29076

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aobolensk
Copy link
Contributor

@aobolensk aobolensk commented Feb 19, 2025

Details:

  • Move convolution with bias transformation from CPU graph optimizer to transformations

Tickets:

  • N/A

@github-actions github-actions bot added category: CPU OpenVINO CPU plugin category: transformations OpenVINO Runtime library - Transformations labels Feb 19, 2025
@github-actions github-actions bot added the category: Core OpenVINO Core (aka ngraph) label Feb 21, 2025
@aobolensk aobolensk force-pushed the fuse-bias branch 9 times, most recently from 2c87699 to ce6fcca Compare February 27, 2025 19:51
@aobolensk aobolensk marked this pull request as ready for review February 27, 2025 20:05
@aobolensk aobolensk requested review from a team as code owners February 27, 2025 20:05
@aobolensk aobolensk requested review from itikhono and removed request for a team February 27, 2025 20:05
@@ -51,5 +54,8 @@ std::vector<TRShape> shape_infer(const TOp* op,
return output_shapes;
}
} // namespace v1

using v1::shape_infer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could using be avoided in .hpp file? How complex is alternative solution?
fyi @praasz

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be removed or have some local scope only like: function, code block etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted, the only drawback is the change of 1 line in GPU plugin

@CuriousPanCake
Copy link
Contributor

Are there going to be any tests added for the added functionality?

namespace op {
namespace internal {

class TRANSFORMATIONS_API ConvolutionBiased : public ov::op::util::ConvolutionFwdPropBase {
Copy link
Member

Choose a reason for hiding this comment

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

TRANSFORMATIONS_API is usually used for exporting transformations, not operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, this is strange. I can see TRANSFORMATIONS_API macro usage in this context across many files in src/common/transformations/include/ov_ops dir. Do they also use that in incorrect way?

Copy link
Member

Choose a reason for hiding this comment

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

I see, then we may re-do this in separate PR. Operation is not a transformation:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be TRANSFORMATIONS_API as it regards build target, not class category.

@aobolensk aobolensk requested review from zKulesza and removed request for a team March 10, 2025 09:09
@github-actions github-actions bot added the category: docs OpenVINO documentation label Mar 10, 2025
Copy link
Contributor

@t-jankowski t-jankowski left a comment

Choose a reason for hiding this comment

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

Ok for core part.

@dmitry-gorokhov
Copy link

dmitry-gorokhov commented Mar 11, 2025

@aobolensk General comment for this PR:
I would say I totally don't see any sence in separate ConvolutionBiased operation. Internal opset serves as opset aligned with HW plugins needs:

  1. Several operations from external opset are executed via single node (so the expectation is to have single internal op)
  2. Operations are extended with additional semantics (like quantization or weights compression params).

Item 1 is especially important to avoid cartesian product of operation types in bounds of item 2.
Please look at #26239 as an example of aligned internal opset extension.

So applying it for Convolution I would say we need to have 2 Internal operations:

  1. internal::Convolution (with bias and groups (to cover GroupedConvolution case))
  2. internal::ConvolutionQuantized (internal::Convolution + quantization params)

GPU plugin already support such semantics inplemented as intel_gpu::op::Convolution (src/plugins/intel_gpu/include/intel_gpu/op/convolution.hpp).
My proposal is to extract GPU impl (both operation and corresponding transformation) into common internal opset and reuse between CPU and GPU. This should be done in 2 separate stage: first will be internal::Convolution and second - internal::ConvolutionQuantized.

@aobolensk aobolensk force-pushed the fuse-bias branch 6 times, most recently from 447e18b to 14089a2 Compare March 12, 2025 15:13
#include "itt.hpp"
#include "openvino/op/util/precision_sensitive_attribute.hpp"

using namespace std;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Comment on lines 52 to 53
const auto& bias_et = get_input_element_type(2);
result_et = bias_et;
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 auto& bias_et = get_input_element_type(2);
result_et = bias_et;
result_et = get_input_element_type(2);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


using namespace std;

namespace ov {
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
namespace ov {
namespace ov::op::internal {

Just optional detail to consider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


const auto output_shapes = op::shape_infer(this, input_shapes, m_pads_begin, m_pads_end);
set_output_type(0, result_et, output_shapes[0]);
set_num_spatial(num_spatial, input_shapes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be set if value is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please clarify what value do you mean here?

Copy link
Contributor

Choose a reason for hiding this comment

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

In case when num_spatial == util::num_spatial_undefined?
Maybe it should be set only when is different than num_spatial_undefined?

template <class TOp,
class TShape,
class TRShape = result_shape_t<TShape>,
typename std::enable_if<std::is_same<TOp, internal::Convolution>::value>::type* = nullptr>
Copy link
Contributor

Choose a reason for hiding this comment

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

the enable_if shoudl not be required just use internal::Convolution as type

@aobolensk aobolensk force-pushed the fuse-bias branch 2 times, most recently from 9cbf58e to 53607e1 Compare March 17, 2025 08:11
@github-actions github-actions bot removed the category: GPU OpenVINO GPU plugin label Mar 17, 2025
@@ -0,0 +1,57 @@
// Copyright (C) 2025 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) 2025 Intel Corporation
// Copyright (C) 2018-2025 Intel Corporation

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

@aobolensk aobolensk force-pushed the fuse-bias branch 3 times, most recently from a2bf6e8 to 97c687e Compare March 21, 2025 08:36
Copy link
Contributor

This PR will be closed in a week because of 2 weeks of no activity.

@github-actions github-actions bot added the Stale label Apr 14, 2025
@aobolensk aobolensk force-pushed the fuse-bias branch 3 times, most recently from acfbc8d to b5bd157 Compare April 17, 2025 17:22
@github-actions github-actions bot removed the Stale label Apr 18, 2025
}

int64_t groups = -1;
auto weights_shape = gconv->get_input_partial_shape(1);
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
auto weights_shape = gconv->get_input_partial_shape(1);
const auto& weights_shape = gconv->get_input_partial_shape(1);

#include "ov_ops/convolution.hpp"
#include "transformations/utils/utils.hpp"

static inline std::vector<size_t> getNormalizedDimsBySize(const std::vector<size_t>& dims, size_t ndims) {
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
static inline std::vector<size_t> getNormalizedDimsBySize(const std::vector<size_t>& dims, size_t ndims) {
static inline std::vector<size_t> getNormalizedDimsBySize(const ov::Shape& dims, size_t ndims) {

ov::NodeVector new_ops;

std::shared_ptr<ov::Node> final_bias = bias;
auto add_shape = add->get_output_partial_shape(0);
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
auto add_shape = add->get_output_partial_shape(0);
const auto& add_shape = add->get_output_partial_shape(0);


ov::pass::ConvolutionBiasFusion::ConvolutionBiasFusion() {
MATCHER_SCOPE(ConvolutionBiasFusion);
using namespace ov::pass::pattern;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Core OpenVINO Core (aka ngraph) category: CPU OpenVINO CPU plugin category: docs OpenVINO documentation category: transformations OpenVINO Runtime library - Transformations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants