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] Fix reset output issue #27695

Merged
merged 9 commits into from
Dec 2, 2024
Merged

[GPU] Fix reset output issue #27695

merged 9 commits into from
Dec 2, 2024

Conversation

ahnyoung-paul
Copy link
Contributor

@ahnyoung-paul ahnyoung-paul commented Nov 22, 2024 •
edited
Loading

Details:

  • Fix invalid output memory usages by reset output memory
    • At the previous iteration, gather primitive has output memory which is refer input memory because it is optimized out but it also use input memory as output without reallocation memory at the next iteration which turns to executed from optimized out. it makes memory corruption issue.
    • Reset output memory when the current prim status is changed from optimized / skipped to executed
      image

Tickets:

  • 154591

@ahnyoung-paul ahnyoung-paul added category: GPU OpenVINO GPU plugin pr: needs tests PR needs tests updating labels Nov 22, 2024
@ahnyoung-paul ahnyoung-paul requested review from a team as code owners November 22, 2024 10:11
@ahnyoung-paul ahnyoung-paul changed the title … [GPU] Fix reset output issue Nov 25, 2024
@ahnyoung-paul ahnyoung-paul force-pushed the fix_reset_output_mem_issue branch from 37b7f8a to be2f05e Compare November 25, 2024 11:54
@ahnyoung-paul ahnyoung-paul removed the pr: needs tests PR needs tests updating label Nov 25, 2024
@ahnyoung-paul ahnyoung-paul force-pushed the fix_reset_output_mem_issue branch 2 times, most recently from 4d2ad88 to e50b408 Compare November 25, 2024 12:32
yeonbok
yeonbok reviewed Nov 27, 2024
View reviewed changes
src/plugins/intel_gpu/src/graph/include/primitive_inst.h Outdated
@@ -403,6 +403,7 @@ class primitive_inst {
bool _can_share_buffer = true;
bool _is_constant = false;
bool _needs_completion_event = false;
bool _no_execution_prev = false;
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 is used only in prepare_primitive & realloc_if_needed, , could you please remove this from member of primitive_inst, and declare it as a local variable in prepare_primitive, and pass it as function argument of realloc_if_neede?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, about the name of the variable, how about prev_execution_skipped?

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 it. please review it again.

yeonbok
yeonbok reviewed Nov 27, 2024
View reviewed changes
src/plugins/intel_gpu/src/graph/primitive_inst.cpp Outdated
}
}
_outputs[0] = nullptr;
_max_output_layout_count[0] = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

These two are being used here and there. Could you refactor this as a member function of primitive_inst? (e.g., clear_output_memory)

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 it. please review it again.

@ahnyoung-paul ahnyoung-paul force-pushed the fix_reset_output_mem_issue branch 4 times, most recently from e5d821b to 660a00c Compare November 29, 2024 08:30
yeonbok
yeonbok reviewed Nov 29, 2024
View reviewed changes
src/plugins/intel_gpu/src/graph/graph_optimizer/mark_runtime_skippable_nodes.cpp Outdated
@@ -47,6 +47,7 @@ void mark_runtime_skippable_nodes::run(program& p) {
!node->is_type<non_max_suppression_gather>()) {
// always to skip, no runtime execution
node->can_be_optimized(true);
node->set_runtime_skippable(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned offline, this is against the original intention of this code block.
Instead, I think we should fix the do_runtime_skip_xx functions to just return when the node is not "runtime_skippable"

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 see. I removed this code and used is_runtime_skippable instead of can_be_optimized in do_runtime_skip_xxx.
please review it again. thanks.

@yeonbok yeonbok added this pull request to the merge queue Dec 2, 2024
Merged via the queue into openvinotoolkit:master with commit cfe2a59 Dec 2, 2024
155 checks passed
@yeonbok yeonbok deleted the fix_reset_output_mem_issue branch December 2, 2024 05:38
@ahnyoung-paul ahnyoung-paul mentioned this pull request Dec 3, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Dec 3, 2024
@ahnyoung-paul
Fix reset output memory issue (#27853)
662a2e6 
### Details:
 - *Fix accuracy issue for reset output memory of optimized prim*
 - original PRs:
     - #27695
     - #27439
     - #27517


### Tickets:
 - *154591*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yeonbok yeonbok yeonbok approved these changes

Assignees
No one assigned
Labels
category: GPU OpenVINO GPU plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ahnyoung-paul @yeonbok