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] Optimize out Gather by converting to implicit crop #17743

Merged
merged 1 commit into from
Jun 19, 2023

Conversation

byungilm
Copy link
Contributor

  • Changed Gather if it cropped input along batch
  • Converted Gather to cldnn Crop in CreateGatherOpBase

Details:

  • Converted Gather to cldnn Crop in CreateGatherOpBase in ops/gather
  • Optimized Crop as implicit crop

Tickets:

  • 104425

@byungilm byungilm requested review from a team as code owners May 25, 2023 23:09
@github-actions github-actions bot added the category: GPU OpenVINO GPU plugin label May 25, 2023
@byungilm byungilm force-pushed the opt_out_gather_crop branch from 7d9919b to 8848ebd Compare May 31, 2023 10:06
@byungilm byungilm requested review from isanghao and hyunback May 31, 2023 10:14
Copy link
Contributor

@isanghao isanghao 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 in general. let me check performance.

if (usr->get_preferred_impl_type() == impl_types::onednn)
return;
}
if (is_optimizable_padding_for_crop(node)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about style below:

if (!is_optimizable_padding_for_crop)())
   return;

Other code uses such style and it keeps indent small.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied

if (is_optimizable_padding_for_crop(node)) {
const auto& crop_layout = node.get_output_layout();
const auto& crop_size = crop_layout.get_tensor();
const auto& out_padd = crop_layout.data_padding;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: padd -> pad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified. Also applied to old code.

auto new_padding = out_padd;
new_padding.lower_size().batch[0] = opt_lower_pad;
new_padding.upper_size().batch[0] = opt_upper_pad;
node.set_output_padding(new_padding);
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 unittest for this.

@byungilm byungilm force-pushed the opt_out_gather_crop branch 3 times, most recently from 4c370b6 to 6b96b85 Compare June 6, 2023 23:37
@byungilm byungilm force-pushed the opt_out_gather_crop branch 4 times, most recently from 2e6186d to 356bbdc Compare June 14, 2023 16:51
@byungilm byungilm requested a review from isanghao June 14, 2023 16:53

std::vector<int8_t> expected_results = {
5, 6, 7, 8
};
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of this test? It is just generated as gather, not crop..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I explained shortly, I wanted to check processing of B axis cropping by both gather and crop.
I also added similar test into crop_gpu_test.
Expected results of can_be_optimized are different.

@byungilm byungilm force-pushed the opt_out_gather_crop branch from 28c0955 to 9d4d38e Compare June 15, 2023 06:06
@byungilm byungilm requested a review from isanghao June 15, 2023 06:16

std::vector<int8_t> expected_results = {
5, 6, 7, 8
};
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add crop test for opt-out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test also pass through the logic.
[ RUN ] crop_single_axis.simple_Baxis
-- prepare_buffer_fusing::run
-- can_crop_be_optimized_along_batch()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test into 'prepare_buffer_fusing'

@byungilm byungilm force-pushed the opt_out_gather_crop branch from 9d4d38e to 28dc744 Compare June 16, 2023 02:16
+ Changed Gather if it divides input tensor along batch axis
+ Converted Gather to cldnn Crop in CreateGatherOpBase
+ Added implicit Crop condition for batch axis

Signed-off-by: Min, Byungil <[email protected]>
@byungilm byungilm force-pushed the opt_out_gather_crop branch from 28dc744 to 8f35c81 Compare June 16, 2023 08:56
@isanghao isanghao enabled auto-merge (squash) June 16, 2023 09:21
@isanghao isanghao merged commit 555c083 into openvinotoolkit:master Jun 19, 2023
alvoron pushed a commit to alvoron/openvino that referenced this pull request Jun 21, 2023
…lkit#17743)

+ Changed Gather if it divides input tensor along batch axis
+ Converted Gather to cldnn Crop in CreateGatherOpBase
+ Added implicit Crop condition for batch axis

Signed-off-by: Min, Byungil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: GPU OpenVINO GPU plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants