Skip to content
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

[cpu] Remove custom shape inference factories #27924

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

praasz
Copy link
Contributor

@praasz praasz commented Dec 5, 2024

Details:

  • Remove custom shape inference factories CPU nodes.

Related PR

Tickets:

@praasz praasz added this to the 2025.0 milestone Dec 5, 2024
@praasz praasz requested review from a team as code owners December 5, 2024 08:33
@github-actions github-actions bot added the category: CPU OpenVINO CPU plugin label Dec 5, 2024
to inputs data as constant if available

Signed-off-by: Raasz, Pawel <[email protected]>
@praasz praasz force-pushed the shape-infer/remove-cpu-custom-shape-infer-factories branch from 14ee211 to 75dfef5 Compare December 9, 2024 10:38
Comment on lines +383 to +424
/**
* @brief Shape inference for v14 MaxPool.
*
* It requires dedicated port mask for CPU output shape data dependency but is not used by inference function.
* Review shape_infer function to include this dependency.
*/
template <>
class ShapeInferPaddingTA<ov::op::v14::MaxPool, EMPTY_PORT_MASK> : public ShapeInferPaddingBase {
public:
using ShapeInferPaddingBase::ShapeInferPaddingBase;

ov::optional<std::vector<StaticShape>> infer(const std::vector<StaticShapeRef>& input_shapes,
const ov::ITensorAccessor&) override {
return {shape_infer(static_cast<ov::op::v14::MaxPool*>(m_node.get()), input_shapes, m_pads_begin, m_pads_end)};
}

port_mask_t get_port_mask() const override {
return util::bit::mask(0);
}
};

/**
* @brief Shape inference for v8 MaxPool.
*
* It requires dedicated port mask for CPU output shape data dependency but is not used by inference function.
* Review shape_infer function to include this dependency.
*/
template <>
class ShapeInferPaddingTA<ov::op::v8::MaxPool, EMPTY_PORT_MASK> : public ShapeInferPaddingBase {
public:
using ShapeInferPaddingBase::ShapeInferPaddingBase;

ov::optional<std::vector<StaticShape>> infer(const std::vector<StaticShapeRef>& input_shapes,
const ov::ITensorAccessor&) override {
return {shape_infer(static_cast<ov::op::v8::MaxPool*>(m_node.get()), input_shapes, m_pads_begin, m_pads_end)};
}

port_mask_t get_port_mask() const override {
return util::bit::mask(0);
}
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Pooling::Pooling CPU node has been using NgraphShapeInferFactory without any additional mask modifications. And it seems that the Pooling node doesn't require input data dependency for shape inference. Such change may negatively impact performance, as the artificial data dependency introduce additional synchronizations to inference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no issue when pooling is created as Pooling node.
The issue is visible if is not possible to make as Pooling and the Reference node, its case for some dynamic cases
in tests/layer_tests/pytorch_tests/test_pooling.py::TestPooling tests.

The previous Reference node always used FULL_MASK regardless if shape inference is defined or not (fallback path). I think this approach has masked issues. Now reference use mask defined in shape inference (if defined) or FULL_MASK for fallback path.

Then issue appears in when the input(0) has dynamic shape but data at port(0) has static shape (exception thrown when get static shape). When dada dependency is set then static shape from data is taken and pooling output shape can be calculated.

These specialization (can be removed if problem solved) shows that issue could be fixed in:

  • shape inference add additional data dependency (it looks like is not required now)
  • for dynamic cases the data dependency should be always set for port 0 to get input static shape.

For FakeQuantize is similar issue and there are test failing if data dependency not set to port 0

@praasz praasz requested a review from maxnick December 16, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: CPU OpenVINO CPU plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants