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]: Added ROIAlignRotated basic impl. #23844

Merged

Conversation

pkowalc1
Copy link
Contributor

@pkowalc1 pkowalc1 commented Apr 3, 2024

Details:

  • Added basic implementation of ROIAlignRotated
  • It uses implementation from core::reference - the goal was just to add support for the op, not the optimized implementation.
  • No unit tests added, since impl is already tested by core::reference and functional tests.

Tickets:

pkowalc1 and others added 30 commits March 14, 2024 11:06
@github-actions github-actions bot removed the category: build OpenVINO cmake script / infra label May 13, 2024
src/core/src/op/roi_align_rotated.cpp Outdated Show resolved Hide resolved
src/core/src/op/roi_align_rotated.cpp Outdated Show resolved Hide resolved
src/core/src/op/roi_align_rotated.cpp Outdated Show resolved Hide resolved
src/core/src/op/roi_align_rotated.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added category: CPU OpenVINO CPU plugin category: build OpenVINO cmake script / infra labels May 14, 2024
@mlukasze mlukasze added this to the 2024.2 milestone May 15, 2024
@pkowalc1 pkowalc1 added this pull request to the merge queue May 15, 2024
Merged via the queue into openvinotoolkit:master with commit cf5bf71 May 15, 2024
115 checks passed
@pkowalc1 pkowalc1 deleted the roi_align_rotated_intel_cpu_plugin_impl branch May 15, 2024 13:52

#include "roi_align_rotated.h"

#include <openvino/opsets/opset14.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include just required operation headers.

namespace ov {
namespace intel_cpu {
namespace node {

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add fn isSupportedOperation(), like in other CPU nodes

ov::element::Type outputPrec = getOriginalOutputPrecisionAtPort(0);

addSupportedPrimDesc(
{{LayoutType::ncsp, inputPrec0}, {LayoutType::ncsp, ov::element::f32}, {LayoutType::ncsp, ov::element::i32}},
Copy link
Contributor

Choose a reason for hiding this comment

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

As per specification, 1st and 2nd inputs have the same precisions. Why the second one is always f32?
Also need to check this value. Plugin supports only f32, f16 and bf16 floating point types.


void ROIAlignRotated::execute(dnnl::stream) {
const ov::element::Type type = getOriginalInputPrecisionAtPort(0);
executeImpl<ov::element::f32>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this fn is called twice?

CASE(bf16);
CASE(f16);
CASE(f32);
CASE(f64);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not supported by plugin

@nshchego
Copy link
Contributor

Please add CPU layer tests

@@ -186,6 +186,11 @@ if (ENABLE_SNIPPETS_LIBXSMM_TPP)
target_include_directories(${TARGET_NAME} SYSTEM PRIVATE $<TARGET_PROPERTY:xsmm,INCLUDE_DIRECTORIES>)
endif ()
target_include_directories(${TARGET_NAME} SYSTEM PRIVATE $<TARGET_PROPERTY:dnnl,INCLUDE_DIRECTORIES>)

# Temporal solution to use template reference implementations in cases where optimizied implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a ticket number

Copy link
Contributor

Choose a reason for hiding this comment

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

executeImpl<ov::element::OV_TYPE>(); \
break;

switch (type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use OV_SWITCH instead

github-merge-queue bot pushed a commit that referenced this pull request Jul 3, 2024
### Details:
 - Register `Col2im-15` in CPU Plugin
- Use reference implementation, used
#23844 as reference
 - Add tests

### Tickets:
 - CVS-142438

### Related PRs:
 - #24548
 - #24197
 - #23947
 - #24569

---------

Co-authored-by: Michal Lukaszewski <[email protected]>
Co-authored-by: Maksim Kutakov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: build OpenVINO cmake script / infra category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings category: CPU OpenVINO CPU plugin category: IE Tests OpenVINO Test: plugins and common category: ONNX FE OpenVINO ONNX FrontEnd Code Freeze
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants