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

aobolensk · 2025-02-19T18:21:13Z

Details:

Move convolution with bias transformation from CPU graph optimizer to transformations

Tickets:

N/A

t-jankowski · 2025-02-28T13:51:04Z

src/core/shape_inference/include/convolution_shape_inference.hpp

@@ -51,5 +54,8 @@ std::vector<TRShape> shape_infer(const TOp* op,
    return output_shapes;
 }
 }  // namespace v1
+
+using v1::shape_infer;


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

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

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

CuriousPanCake · 2025-03-03T10:11:02Z

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

rkazants · 2025-03-06T10:20:48Z

src/common/transformations/include/ov_ops/convolution_biased.hpp

+namespace op {
+namespace internal {
+
+class TRANSFORMATIONS_API ConvolutionBiased : public ov::op::util::ConvolutionFwdPropBase {


TRANSFORMATIONS_API is usually used for exporting transformations, not operations.

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?

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

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

rkazants · 2025-03-06T10:23:14Z

src/common/transformations/include/transformations/op_conversions/convert_convolution.hpp

+
+}  // namespace pass
+}  // namespace ov
+


please add description for this transofrmation

Where it is better to add such description? This is transformation from convolution of the particular opset to the convolution of the internal opset

rkazants · 2025-03-06T10:25:14Z

src/plugins/intel_cpu/src/nodes/conv.cpp

@@ -266,8 +270,35 @@ Convolution::Convolution(const std::shared_ptr<ov::Node>& op, const GraphContext

    auto convolutionOp = ov::as_type_ptr<ov::op::v1::Convolution>(op);
    auto groupConvolutionOp = ov::as_type_ptr<ov::op::v1::GroupConvolution>(op);
+    auto biasedConvolutionOp = ov::as_type_ptr<ov::op::internal::ConvolutionBiased>(op);


Shouldn't we add specification for this operation here: https://github.com/openvinotoolkit/openvino/tree/master/docs/articles_en/documentation/openvino-ir-format/operation-sets/operation-specs/internal ?

That's a good question. Shall we? if it is a regular convolution, but with bias input supported

aobolensk · 2025-03-10T09:03:09Z

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

Added unit test for the new operation. Functional tests already check whether node is fused or not. Basically, here we have the situation that this pass was disabled in graph_optimizer and enabled on transformation side. So current functional and model tests play the role of regression tests that transformation was moved correctly from one infrastructure to another

t-jankowski

Ok for core part.

dmitry-gorokhov · 2025-03-11T13:12:21Z

@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:

Several operations from external opset are executed via single node (so the expectation is to have single internal op)
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:

internal::Convolution (with bias and groups (to cover GroupedConvolution case))
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.

praasz · 2025-03-13T08:20:21Z

src/common/transformations/src/ov_ops/convolution.cpp

+#include "itt.hpp"
+#include "openvino/op/util/precision_sensitive_attribute.hpp"
+
+using namespace std;


praasz · 2025-03-13T08:20:58Z

src/common/transformations/src/ov_ops/convolution.cpp

+        const auto& bias_et = get_input_element_type(2);
+        result_et = bias_et;


Suggested change

const auto& bias_et = get_input_element_type(2);

result_et = bias_et;

result_et = get_input_element_type(2);

praasz · 2025-03-13T08:23:48Z

src/common/transformations/src/ov_ops/convolution.cpp

+
+using namespace std;
+
+namespace ov {


Suggested change

namespace ov {

namespace ov::op::internal {

Just optional detail to consider

praasz · 2025-03-13T08:24:40Z

src/common/transformations/src/ov_ops/convolution.cpp

+
+    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);


Should be set if value is undefined?

praasz · 2025-03-13T08:26:07Z

...ormations/src/transformations/op_conversions/convert_convolution_to_convolution_internal.cpp

+#include "openvino/pass/pattern/op/wrap_type.hpp"
+#include "ov_ops/convolution.hpp"
+
+using namespace ov;


Can be removed?

praasz · 2025-03-13T09:04:09Z

src/core/shape_inference/include/internal_convolution_shape_inference.hpp

+namespace ov {
+namespace op {


Suggested change

namespace ov {

namespace op {

namespace ov::op::internal {

praasz · 2025-03-13T09:05:03Z

src/core/shape_inference/include/internal_convolution_shape_inference.hpp

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


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

praasz · 2025-03-13T09:05:20Z

src/core/shape_inference/include/convolution_shape_inference.hpp

 #include "utils.hpp"

 namespace ov {
 namespace op {
-namespace v1 {
+


restore v1 also

praasz · 2025-03-13T09:06:57Z

...lugins/intel_cpu/tests/unit/shape_inference_test/convolution_biased_shape_inference_test.cpp

+    const auto pads_end = CoordinateDiff{0, 0, 0};
+    const auto auto_pad = op::PadType::SAME_UPPER;
+
+    const auto data = std::make_shared<op::v0::Parameter>(element::f32, PartialShape{-1, -1, -1, -1, -1});


Suggested change

const auto data = std::make_shared<op::v0::Parameter>(element::f32, PartialShape{-1, -1, -1, -1, -1});

const auto data = std::make_shared<op::v0::Parameter>(element::f32, PartialShape::dynamic(5));

praasz · 2025-03-13T09:08:13Z

...lugins/intel_cpu/tests/unit/shape_inference_test/convolution_biased_shape_inference_test.cpp

+#include "ov_ops/convolution.hpp"
+#include "utils.hpp"
+
+using namespace ov;


Remove it.
There can be some collisions in type e.g. Shape is better to use ov:: for types from core.

aobolensk added do_not_review do_not_merge labels Feb 19, 2025

github-actions bot added category: CPU OpenVINO CPU plugin category: transformations OpenVINO Runtime library - Transformations labels Feb 19, 2025

aobolensk force-pushed the fuse-bias branch from 1eefa01 to fa96b37 Compare February 20, 2025 14:22

github-actions bot added the category: Core OpenVINO Core (aka ngraph) label Feb 21, 2025

aobolensk force-pushed the fuse-bias branch 9 times, most recently from 2c87699 to ce6fcca Compare February 27, 2025 19:51

aobolensk removed do_not_review do_not_merge labels Feb 27, 2025

aobolensk marked this pull request as ready for review February 27, 2025 20:05

aobolensk requested review from a team as code owners February 27, 2025 20:05

aobolensk requested review from itikhono and removed request for a team February 27, 2025 20:05

mlukasze requested review from jane-intel and CuriousPanCake February 28, 2025 05:42

t-jankowski reviewed Feb 28, 2025

View reviewed changes

aobolensk force-pushed the fuse-bias branch from 98f76d9 to fbf6810 Compare March 6, 2025 10:17

rkazants reviewed Mar 6, 2025

View reviewed changes

aobolensk force-pushed the fuse-bias branch 4 times, most recently from b043ab0 to 6f3e01a Compare March 7, 2025 10:34

aobolensk requested review from a team as code owners March 7, 2025 10:52

github-actions bot added the category: GPU OpenVINO GPU plugin label Mar 7, 2025

aobolensk force-pushed the fuse-bias branch from 613c955 to ec3d3ba Compare March 7, 2025 10:57

aobolensk requested a review from a team as a code owner March 10, 2025 09:09

aobolensk requested review from zKulesza and removed request for a team March 10, 2025 09:09

github-actions bot added the category: docs OpenVINO documentation label Mar 10, 2025

aobolensk force-pushed the fuse-bias branch from 9a8744f to 7e62a5e Compare March 10, 2025 09:11

t-jankowski approved these changes Mar 10, 2025

View reviewed changes

aobolensk force-pushed the fuse-bias branch 6 times, most recently from 447e18b to 14089a2 Compare March 12, 2025 15:13

praasz reviewed Mar 13, 2025

View reviewed changes

aobolensk force-pushed the fuse-bias branch from 14089a2 to f0ec8fd Compare March 14, 2025 13:28

aobolensk added 4 commits March 14, 2025 14:30

fuse bias

1b95225

rename convolution biased -> convolution

cf248ac

initial take on groupconv support

f0ec8fd

Address part of review comments

8223972

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aobolensk commented Feb 19, 2025 •

edited

Loading

t-jankowski Feb 28, 2025

praasz Mar 3, 2025

aobolensk Mar 7, 2025

CuriousPanCake commented Mar 3, 2025

rkazants Mar 6, 2025

aobolensk Mar 6, 2025

rkazants Mar 6, 2025

aobolensk Mar 6, 2025

t-jankowski Mar 6, 2025

rkazants Mar 6, 2025

aobolensk Mar 10, 2025

rkazants Mar 6, 2025 •

edited

Loading

aobolensk Mar 7, 2025 •

edited

Loading

aobolensk commented Mar 10, 2025

t-jankowski left a comment

dmitry-gorokhov commented Mar 11, 2025 •

edited

Loading

praasz Mar 13, 2025

praasz Mar 13, 2025

praasz Mar 13, 2025

praasz Mar 13, 2025

praasz Mar 13, 2025

praasz Mar 13, 2025

praasz Mar 13, 2025

praasz Mar 13, 2025

praasz Mar 13, 2025

praasz Mar 13, 2025

		const auto& bias_et = get_input_element_type(2);
		result_et = bias_et;

	const auto& bias_et = get_input_element_type(2);
	result_et = bias_et;
	result_et = get_input_element_type(2);

	const auto data = std::make_shared<op::v0::Parameter>(element::f32, PartialShape{-1, -1, -1, -1, -1});
	const auto data = std::make_shared<op::v0::Parameter>(element::f32, PartialShape::dynamic(5));

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

Are you sure you want to change the base?

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

Conversation

aobolensk commented Feb 19, 2025 • edited Loading

Details:

Tickets:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CuriousPanCake commented Mar 3, 2025

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

rkazants Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

aobolensk Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

aobolensk commented Mar 10, 2025

t-jankowski left a comment

Choose a reason for hiding this comment

dmitry-gorokhov commented Mar 11, 2025 • edited Loading

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

aobolensk commented Feb 19, 2025 •

edited

Loading

rkazants Mar 6, 2025 •

edited

Loading

aobolensk Mar 7, 2025 •

edited

Loading

dmitry-gorokhov commented Mar 11, 2025 •

edited

Loading