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.

Sign up for GitHub

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

Jump to bottom

[GPU] Add new operation GatherElements to IE clDNN plugin #6676

Merged
merged 11 commits into from
Jul 26, 2021
Merged

[GPU] Add new operation GatherElements to IE clDNN plugin #6676

merged 11 commits into from
Jul 26, 2021

Conversation

yunji-yunji
Copy link
Contributor

@yunji-yunji yunji-yunji commented Jul 16, 2021 •
edited by yeonbok
Loading

Description

Added new operation: GatherElements

Tickets:

  • 42636

@yeonbok yeonbok changed the title Add new operation GatherElements to IE clDNN plugin [IE GPU] Add new operation GatherElements to IE clDNN plugin Jul 16, 2021
@vladimir-paramuzov vladimir-paramuzov changed the title [IE GPU] Add new operation GatherElements to IE clDNN plugin [GPU] Add new operation GatherElements to IE clDNN plugin Jul 16, 2021
@vladimir-paramuzov vladimir-paramuzov added the category: GPU OpenVINO GPU plugin label Jul 16, 2021
@vladimir-paramuzov vladimir-paramuzov added this to the 2022.1 milestone Jul 16, 2021
@yeonbok yeonbok marked this pull request as ready for review July 19, 2021 05:28
@yeonbok yeonbok requested review from a team as code owners July 19, 2021 05:28
@yeonbok yeonbok requested a review from a team July 19, 2021 05:28
vladimir-paramuzov
vladimir-paramuzov requested changes Jul 19, 2021
View reviewed changes
inference-engine/src/cldnn_engine/ops/gather_elements.cpp Outdated Show resolved Hide resolved
inference-engine/src/cldnn_engine/ops/gather_elements.cpp Outdated
auto outLayout = DefaultFormatForDims(op->get_output_shape(0).size());

auto primitive = cldnn::gather_elements(layerName,
inputPrimitives[0],
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please align args

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix 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.

Fixed.

inference-engine/src/cldnn_engine/ops/gather_elements.cpp Outdated

REGISTER_FACTORY_IMPL(v6, GatherElements);

} // namespace CLDNNPlugin
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please add newline in eof

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix 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.

Fixed.

inference-engine/thirdparty/clDNN/api/cldnn/primitives/gather_elements.hpp Outdated
@@ -0,0 +1,71 @@
/*
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 new copyright message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix 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.

Fixed.

inference-engine/thirdparty/clDNN/api/cldnn/primitives/gather_elements.hpp Outdated
const primitive_id& indices,
const format& output_format,
const tensor& output_shape,
// const uint8_t axis = 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove dead code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll 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.

Fixed.

inference-engine/thirdparty/clDNN/kernel_selector/core/cl_kernels/gather_elements_ref.cl Outdated
size_t outer_sum = (out_idx / outer_sum_inc_indices) * outer_sum_inc_data;
size_t inner_sum = out_idx % max_inner_sum;
if (indices[out_idx] < 0 || indices[out_idx] >= data_shape[AXIS]) {
printf("indices values of GatherElement exceed data size.\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

remove debug code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll 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.

Fixed.

inference-engine/thirdparty/clDNN/kernel_selector/core/cl_kernels/gather_elements_ref.cl Outdated

#define GET_UPDATES_INDEX(prefix, idx_order) CAT(prefix, _GET_INDEX)(idx_order)

KERNEL(gather_nd_ref)(const __global INPUT0_TYPE* data,
Copy link
Contributor

Choose a reason for hiding this comment

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

gather_nd_ref -> gather_elements_ref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix 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.

Fixed.

inference-engine/tests/functional/plugin/shared/include/single_layer_tests/gather_elements.hpp
#include "shared_test_classes/single_layer/gather_elements.hpp"

namespace LayerTestsDefinitions {

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it empty? I assume here should be test definition like TEST_P(GatherElementsLayerTest, CompareWithRefs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That test definition is already in inference-engine/tests/functional/shared_test_classes/src/single_layer/gather_elements.cpp file.

So I didn't add that code to avoid dupicate declarations.

Copy link
Contributor

Choose a reason for hiding this comment

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

@iefode What is the expected location of test definitions? I see that most of the layers have test definition in inference-engine/tests/functional/plugin/shared/include/single_layer_tests/*.hpp, so I'd expect that gather_elements will be defined in similar way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the location of the test definition from inference-engine/tests/functional/shared_test_classes/src/single_layer/gather_elements.cpp to inference-engine/tests/functional/plugin/shared/include/single_layer_tests/gather_elements.hpp.

And, Fixed header in inference-engine/tests/functional/plugin/cpu/shared_tests_instances/single_layer_tests/gather_elements.cpp file.

@yunji-yunji yunji-yunji force-pushed the gather_elements branch 5 times, most recently from 4769f32 to 7d1334b Compare July 25, 2021 12:56
@yunji-yunji yunji-yunji force-pushed the gather_elements branch 2 times, most recently from dfd6ef9 to d36aa7e Compare July 26, 2021 01:53
lznamens
lznamens reviewed Jul 26, 2021
View reviewed changes
inference-engine/thirdparty/clDNN/api/cldnn/primitives/gather_elements.hpp Outdated
/// @param output_shape Output shape.
/// @param axis Gathering axis.
gather_elements(const primitive_id& id,
const primitive_id& data,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: alignment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it.

inference-engine/thirdparty/clDNN/tests/test_cases/gather_elements_gpu_test.cpp Outdated
FLOAT16(2), FLOAT16(10), FLOAT16(7), FLOAT16(3), FLOAT16(3), FLOAT16(10), FLOAT16(6), FLOAT16(1),
};

DoTest(engine,input0, input1, expected_results, tensor(2, 2, 8, 3), axis);
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 space after "engine," here and in other calls of the function DoTest.

DoTest(engine, input0, input1, expected_results, tensor(2, 2, 8, 3), axis);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed them all.

lznamens
lznamens approved these changes Jul 26, 2021
View reviewed changes
Copy link
Contributor

@lznamens lznamens left a comment

Choose a reason for hiding this comment

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

This PR looks good to me.

kelvinchoi-intel
kelvinchoi-intel reviewed Jul 26, 2021
View reviewed changes
...ne/tests/functional/plugin/cpu/shared_tests_instances/single_layer_tests/gather_elements.cpp Outdated Show resolved Hide resolved
...ne/tests/functional/plugin/gpu/shared_tests_instances/single_layer_tests/gather_elements.cpp Outdated Show resolved Hide resolved
...ne/tests/functional/plugin/gpu/shared_tests_instances/single_layer_tests/gather_elements.cpp Show resolved Hide resolved
inference-engine/thirdparty/clDNN/api/cldnn/primitives/gather_elements.hpp Outdated Show resolved Hide resolved
inference-engine/thirdparty/clDNN/kernel_selector/common/common_types.h Outdated Show resolved Hide resolved
...hirdparty/clDNN/kernel_selector/core/actual_kernels/gather/gather_elements_kernel_selector.h Outdated Show resolved Hide resolved
...e/thirdparty/clDNN/kernel_selector/core/actual_kernels/gather/gather_elements_kernel_ref.cpp Show resolved Hide resolved
inference-engine/thirdparty/clDNN/kernel_selector/core/cl_kernels/gather_elements_ref.cl Show resolved Hide resolved
inference-engine/thirdparty/clDNN/src/impls/ocl/gather_elements.cpp Outdated Show resolved Hide resolved
inference-engine/thirdparty/clDNN/tests/test_cases/fusings_gpu_test.cpp Show resolved Hide resolved
kelvinchoi-intel
kelvinchoi-intel approved these changes Jul 26, 2021
View reviewed changes
Copy link
Contributor

@kelvinchoi-intel kelvinchoi-intel left a comment

Choose a reason for hiding this comment

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

Looks good for me

vladimir-paramuzov
vladimir-paramuzov approved these changes Jul 26, 2021
View reviewed changes
inference-engine/tests/functional/plugin/shared/include/single_layer_tests/gather_elements.hpp
#include "shared_test_classes/single_layer/gather_elements.hpp"

namespace LayerTestsDefinitions {

Copy link
Contributor

Choose a reason for hiding this comment

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

@iefode What is the expected location of test definitions? I see that most of the layers have test definition in inference-engine/tests/functional/plugin/shared/include/single_layer_tests/*.hpp, so I'd expect that gather_elements will be defined in similar way.

...ne/tests/functional/plugin/cpu/shared_tests_instances/single_layer_tests/gather_elements.cpp Outdated
@@ -73,4 +73,4 @@ INSTANTIATE_TEST_SUITE_P(smoke_set5, GatherElementsLayerTest,
::testing::ValuesIn(iPrecisions),
::testing::Values(CommonTestUtils::DEVICE_CPU)),
GatherElementsLayerTest::getTestCaseName);
} // namespace
} // namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

revert it please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll create new PR for this.

inference-engine/thirdparty/clDNN/api/cldnn/primitives/gather_elements.hpp Outdated
/// @}
/// @}
/// @}
} // namespace cldnn
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please add new line in eof (here and in other places). I'd recommend to setup your IDE to add it automatically on save

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for telling me. I added new line from the last commit.

...ine/thirdparty/clDNN/kernel_selector/core/actual_kernels/gather/gather_elements_kernel_ref.h Outdated

GatherElementsAxis axis;

virtual ParamsKey GetParamsKey() const { return base_params::GetParamsKey(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

If this method doesn't change the behavior of base one, then you should not define it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it.

@vladimir-paramuzov vladimir-paramuzov merged commit f5666fb into openvinotoolkit:master Jul 26, 2021
@yunji-yunji yunji-yunji mentioned this pull request Jul 27, 2021
Closed
rnugmanx pushed a commit to rnugmanx/openvino that referenced this pull request Aug 26, 2021
@yunji-yunji
[GPU] Add new operation GatherElements to IE clDNN plugin (openvinoto…
fd9214e 
…olkit#6676)
andrei-cv pushed a commit to andrei-cv/openvino that referenced this pull request Aug 30, 2021
@yunji-yunji @andrei-cv
[GPU] Add new operation GatherElements to IE clDNN plugin (openvinoto…
5395c84 
…olkit#6676)
akuporos pushed a commit to akuporos/openvino that referenced this pull request Sep 29, 2021
@yunji-yunji @akuporos
[GPU] Add new operation GatherElements to IE clDNN plugin (openvinoto…
765124e 
…olkit#6676)
jinhopark8345 added a commit to jinhopark8345/openvino that referenced this pull request Oct 1, 2021
@jinhopark8345
Revert "[GPU] Add new operation GatherElements to IE clDNN plugin (op…
a27d55a 
…envinotoolkit#6676)"

This reverts commit f5666fb.
si-eun-kim added a commit to si-eun-kim/openvino that referenced this pull request Oct 1, 2021
@si-eun-kim
Revert "[GPU] Add new operation GatherElements to IE clDNN plugin (op…
9197511 
…envinotoolkit#6676)"

This reverts commit f5666fb.
yeonbok added a commit to yeonbok/openvino that referenced this pull request Mar 11, 2022
@yeonbok
For handson
19a7c03 
Revert "[GPU] Add new operation GatherElements to IE clDNN plugin (openvinotoolkit#6676)"

This reverts commit f5666fb.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yeonbok yeonbok yeonbok left review comments

@kelvinchoi-intel kelvinchoi-intel kelvinchoi-intel approved these changes

@lznamens lznamens lznamens approved these changes

@vladimir-paramuzov vladimir-paramuzov vladimir-paramuzov approved these changes

@geunhwan geunhwan Awaiting requested review from geunhwan

@ahnyoung-paul ahnyoung-paul Awaiting requested review from ahnyoung-paul

@andrew-k-park andrew-k-park Awaiting requested review from andrew-k-park

Assignees

@vladimir-paramuzov vladimir-paramuzov

Labels
category: GPU OpenVINO GPU plugin
Projects
None yet
Milestone
2022.1
Development

Successfully merging this pull request may close these issues.
5 participants
@yunji-yunji @vladimir-paramuzov @yeonbok @lznamens @kelvinchoi-intel