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] Add check of multiple axis broadcasting in is_valid_fusion() when dynamic shape #28252

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

Conversation

wilson-seok
Copy link
Contributor

Details:

  • Add check of multiple axis broadcasting in is_valid_fusion() when dynamic shape
  • Current ocl fused_op jit constant doesn't support multiple axis boradcase.
    image

Tickets:

  • 159939

@wilson-seok wilson-seok requested review from a team as code owners January 2, 2025 13:23
@github-actions github-actions bot added the category: GPU OpenVINO GPU plugin label Jan 2, 2025
@p-durandin
Copy link
Contributor

Would be good to add test for this case

@p-durandin p-durandin added the pr: needs tests PR needs tests updating label Jan 2, 2025
@@ -2658,6 +2658,19 @@ bool primitive_inst::is_valid_fusion() const {
if (fd.is_type<eltwise>())
can_broadcast = ov::PartialShape::broadcast_merge_into(merged_shape, outer_dep_pshape, fd.typed_desc<eltwise>()->broadcast_spec);

// Check if broadcast happens more than single axis.
// Current FUSED_OP_LOAD macro cannot support broadcast on dynamic dimension.
if (can_broadcast == true && (merged_shape.is_static() && outer_dep_pshape.is_static()) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these two conditions needed? At runtime, shapes should be static always.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I removed it. Thanks!

// Check if broadcast happens more than single axis.
// Current FUSED_OP_LOAD macro cannot support broadcast on dynamic dimension.
if (can_broadcast == true && (merged_shape.is_static() && outer_dep_pshape.is_static()) &&
outer_dep.first->_is_dynamic == true && merged_shape.rank().get_length() == outer_dep_pshape.rank().get_length()) {
Copy link
Contributor

@yeonbok yeonbok Jan 2, 2025

Choose a reason for hiding this comment

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

outer_dep.first->_is_dynamic

Do we need this condition? even though it is not dynamic, the fusion is not allowed isn't it? And also this function is not called for static primitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. the is_valid_fusion() is called in dynamic shape case. I removed it. Thanks!

@yeonbok
Copy link
Contributor

yeonbok commented Jan 2, 2025

Please add a functional test.

@wilson-seok
Copy link
Contributor Author

Would be good to add test for this case

Added func test. Thanks!

@wilson-seok wilson-seok removed the pr: needs tests PR needs tests updating label Jan 3, 2025
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.

3 participants