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

[CPU] Fill the ellipsis mask in StridedSlice node to match the input rank #22109

Merged

Conversation

maxnick
Copy link
Contributor

@maxnick maxnick commented Jan 11, 2024

Details:

Fix the ellipsis mask preprocessing in the StridedSlice node to avoid out of bounds memory access.

Tickets:

  • 128638

@maxnick maxnick requested review from a team as code owners January 11, 2024 18:24
@github-actions github-actions bot added the category: CPU OpenVINO CPU plugin label Jan 11, 2024
@maxnick maxnick added this to the 2024.0 milestone Jan 11, 2024
@maxnick maxnick added the bug Something isn't working label Jan 11, 2024
@maxnick
Copy link
Contributor Author

maxnick commented Jan 12, 2024

@a-sidorova , could you please review?

Copy link
Contributor

@a-sidorova a-sidorova left a comment

Choose a reason for hiding this comment

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

In general LGTM.

But seems like...

  • as we discussed offline, the current specification doesn't cover ranks of masks, paddings- and other cases. I think we have to prioritize the ticket 90128
  • the current implementation is based on assumption that the masks are mapped on input shape (or output??) starting from first element. If masks and shapes rank are different, we expended masks by empty values. But it's not described in the spec as well. Our implementation is written using different assumptions but not the spec. Moreover the reference implementation is different from plugins (the part of masks and paddings, the work with normalization). It might leads to new bugs. I'm not sure that aligning with the reference is good idea as main solution. Because reference may contain bugs as well until we don't have the fully described specification (I remember that I tried to align our impl with the reference code but I see that some parts in the core have been updated). So I think we have to update the specification to avoid many misunderstandings and possible bugs.

@maxnick
Copy link
Contributor Author

maxnick commented Jan 16, 2024

In general LGTM.

But seems like...

  • as we discussed offline, the current specification doesn't cover ranks of masks, paddings- and other cases. I think we have to prioritize the ticket 90128
  • the current implementation is based on assumption that the masks are mapped on input shape (or output??) starting from first element. If masks and shapes rank are different, we expended masks by empty values. But it's not described in the spec as well. Our implementation is written using different assumptions but not the spec. Moreover the reference implementation is different from plugins (the part of masks and paddings, the work with normalization). It might leads to new bugs. I'm not sure that aligning with the reference is good idea as main solution. Because reference may contain bugs as well until we don't have the fully described specification (I remember that I tried to align our impl with the reference code but I see that some parts in the core have been updated). So I think we have to update the specification to avoid many misunderstandings and possible bugs.

СС @mitruska

@maxnick maxnick merged commit 585a7dc into openvinotoolkit:master Jan 16, 2024
94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working category: CPU OpenVINO CPU plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants