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] Skip reorder opt when its dependency is crop #27547

Merged

Conversation

steve-y
Copy link
Contributor

@steve-y steve-y commented Nov 14, 2024

Details:

  • Skip reorder opt when its dependency is crop

Tickets:

  • 155068

@steve-y steve-y requested review from a team as code owners November 14, 2024 04:24
@steve-y steve-y requested a review from wilson-seok November 14, 2024 04:24
@github-actions github-actions bot added the category: GPU OpenVINO GPU plugin label Nov 14, 2024
@andrew-k-park andrew-k-park added the pr: needs tests PR needs tests updating label Nov 14, 2024
@@ -844,6 +844,8 @@ void prepare_buffer_fusing::run(program& p) {
crop_params->input_offsets[0],
node.get_primitive()->axis,
false);
if (static_cast<bool>(crop_layout.data_padding) && node.get_users().front()->is_type<reorder>())
Copy link
Contributor

Choose a reason for hiding this comment

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

As offline discussion, returning here is dangerous. So this condition must already be done within match function.

@steve-y steve-y changed the title [GPU] Skip crop fusing when it has padding and its user is reorder [DO NOT REVIEW] [GPU] Skip crop fusing when it has padding and its user is reorder Nov 14, 2024
@steve-y steve-y force-pushed the sy/skip_crop_fusing_before_reorder branch from 9e3852a to 0b5aa18 Compare November 15, 2024 04:16
@steve-y steve-y changed the title [DO NOT REVIEW] [GPU] Skip crop fusing when it has padding and its user is reorder [DO NOT REVIEW] [GPU] Skip reorder opt when its dependency is crop Nov 15, 2024
@steve-y steve-y force-pushed the sy/skip_crop_fusing_before_reorder branch from 0b5aa18 to 51ac5f3 Compare November 20, 2024 11:58
@steve-y steve-y requested a review from byungilm November 21, 2024 00:45
@steve-y steve-y force-pushed the sy/skip_crop_fusing_before_reorder branch from 51ac5f3 to 1d15768 Compare November 21, 2024 03:13
@@ -295,6 +295,9 @@ void remove_redundant_reorders::run(program& p) {
auto o_layout = r_node.get_output_layout();
const auto& i_layout = r_node.get_input_layout(0);

if (r_node.get_dependency(0).is_type<crop>())
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we skip reorder optimizing which has crop dep even it does not has any padding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. reorder opt is determined in remove_redundant_reorders and crop padding is determined in prepare_buffer_fusing. And remove_redundant_reorders is executed before prepare_buffer_fusing. So padding info isn't known in remove_redundant_reorders pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

if (!update_implementations || (r_node.get_dependency(0).is_type<crop>() && r_node.get_dependency(0).can_be_optimized())

Copy link
Contributor

Choose a reason for hiding this comment

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

At post optimization graph phase, update_implementations is true.
At this phase, buffer fusing is already applied, so we can check the optimization of 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.

Updated as your comments. Thanks for your updated codes and kind explanations.

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 update_implementations needed to be confined to crop only because the code below the condition should be run sometimes, the condition was updated.

@steve-y steve-y changed the title [DO NOT REVIEW] [GPU] Skip reorder opt when its dependency is crop [GPU] Skip reorder opt when its dependency is crop Nov 21, 2024
@steve-y steve-y force-pushed the sy/skip_crop_fusing_before_reorder branch 3 times, most recently from e746860 to 8d5bdd0 Compare December 9, 2024 00:14
@steve-y steve-y force-pushed the sy/skip_crop_fusing_before_reorder branch from 8d5bdd0 to 0f04110 Compare December 12, 2024 03:28
@yeonbok yeonbok removed the pr: needs tests PR needs tests updating label Dec 17, 2024
@yeonbok yeonbok added this pull request to the merge queue Dec 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 17, 2024
@steve-y steve-y force-pushed the sy/skip_crop_fusing_before_reorder branch 3 times, most recently from 64787a9 to ba177e8 Compare December 21, 2024 01:09
@steve-y steve-y added this pull request to the merge queue Dec 21, 2024
Merged via the queue into openvinotoolkit:master with commit 6e02445 Dec 21, 2024
164 checks passed
@steve-y steve-y deleted the sy/skip_crop_fusing_before_reorder branch December 21, 2024 12:38
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.

4 participants