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

[GPU]: SearchSorted basic implementation. #27356

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

Conversation

pkowalc1
Copy link
Contributor

@pkowalc1 pkowalc1 commented Oct 31, 2024

Added GPU reference Search Sorted implementation will unit and func tests. Kernel supports dynamic shapes.

Details:

  • Fixed a bug in reference implementation, when sorted had exactly one element. Added tests for that case.

Tickets:

@pkowalc1 pkowalc1 added the WIP work in progress label Oct 31, 2024
@pkowalc1 pkowalc1 requested review from a team as code owners October 31, 2024 13:01
@github-actions github-actions bot added the category: GPU OpenVINO GPU plugin label Oct 31, 2024
@pkowalc1 pkowalc1 requested review from a team as code owners November 6, 2024 11:31
@github-actions github-actions bot added the category: IE Tests OpenVINO Test: plugins and common label Nov 6, 2024
@pkowalc1 pkowalc1 requested a review from a team as a code owner November 14, 2024 12:04
@github-actions github-actions bot added the category: Core OpenVINO Core (aka ngraph) label Nov 14, 2024
@pkowalc1 pkowalc1 changed the title WIP: [GPU]: Added stub for SearchSorted WIP: [GPU]: SearchSorted basic implementation. Nov 14, 2024
@pkowalc1 pkowalc1 changed the title WIP: [GPU]: SearchSorted basic implementation. [GPU]: SearchSorted basic implementation. Nov 15, 2024
@pkowalc1 pkowalc1 removed the WIP work in progress label Nov 15, 2024
#define CMP <
#endif

OUTPUT_TYPE binary_search_thread(const INPUT0_TYPE search_val,
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 FUNC() and FUNC_CALL() wrappers for functions to guarantee unique function names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

attach_search_sorted_impl::attach_search_sorted_impl() {
#define ADD_TYPE(type) std::make_tuple(data_types::type, format::bfyx), std::make_tuple(data_types::type, format::bfzyx)

implementation_map<search_sorted>::add(impl_types::ocl,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could you please align these definitions with other implementations? Something like:

    auto types = {
        data_types::f32,
        ...
    };

    auto formats = {
        format::bfyx,
        format::bfzyx
    };
    implementation_map<search_sorted>::add(impl_types::ocl, shape_types::any, search_sorted_impl::create, types, formats);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

validate_inputs_count(op, {2});
auto inputs = p.GetInputInfo(op);
auto roi_align_prim = cldnn::search_sorted(layer_type_name_ID(op), inputs[0], inputs[1], op->get_right_mode());
p.add_primitive(*op, roi_align_prim);
Copy link
Contributor

Choose a reason for hiding this comment

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

As you are using impl_param.desc->output_data_types[0].value_or(data_types::i64); for shape inference, output_data_types have to be specified during primitive creation:

Suggested change
p.add_primitive(*op, roi_align_prim);
p.add_primitive(*op, roi_align_prim);
p.output_data_types = get_output_data_types(op);

Copy link
Contributor Author

@pkowalc1 pkowalc1 Nov 25, 2024

Choose a reason for hiding this comment

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

Fixed, but if it "has to be specified", why did it work before?(unit tests, func tests)???

Copy link
Contributor

@praasz praasz left a comment

Choose a reason for hiding this comment

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

LGTM core part

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: GPU OpenVINO GPU plugin category: IE Tests OpenVINO Test: plugins and common
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants