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

[luci/service] Support StridedSlice dynamic shape inference #13968

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

qsunki
Copy link
Contributor

@qsunki qsunki commented Sep 10, 2024

This supports dynamic shape inference for StridedSlice op,
in case where the input node is dynamic or the begin/end nodes are not const.

ONE-DCO-1.0-Signed-off-by: sunki [email protected]

This partially supports dynamic shape inference for StridedSlice op
when input node has dynamic shape and other nodes are const
and input node has static shape and begin, end nodes are non const.

ONE-DCO-1.0-Signed-off-by: sunki <[email protected]>
@qsunki qsunki added the SSAFY label Sep 10, 2024
@qsunki
Copy link
Contributor Author

qsunki commented Sep 10, 2024

From Draft: #13914
For: #13932
Related: #13697

@qsunki qsunki requested review from shs-park, zetwhite, a team and seanshpark September 10, 2024 04:24
@shs-park
Copy link
Contributor

shs-park commented Sep 10, 2024

This partially supports dynamic shape inference for StridedSlice op when input node has dynamic shape and other nodes are const and input node has static shape and begin, end nodes are non const.

A little bit simpler description 😅


This supports dynamic shape inference for StridedSlice op,
in case where the input node is dynamic or the begin/end nodes are not const.

{
INTERNAL_EXN("StridedSlice strides node are not Constant");
}
// TODO Support cases where the mask attributes are non-zero.
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 need to consider this attributes when begin/end is not const?

I'm asking this question because I don't think there's a need to add this comment here,
if the attribute is not related to the if statement below.

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 thought that when the mask was set, some parts of the shape could be fixed. However, since there are no issues with inference now, it would be better to remove the comment.

shs-park
shs-park previously approved these changes Sep 11, 2024
Copy link
Contributor

@shs-park shs-park left a comment

Choose a reason for hiding this comment

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

LGTM!
=)

@shs-park shs-park added the PR/ready for review It is ready to review. Please review it. label Sep 11, 2024
@shs-park shs-park requested a review from a team September 11, 2024 06:13
Copy link
Contributor

@shs-park shs-park left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@zetwhite zetwhite left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@seanshpark seanshpark left a comment

Choose a reason for hiding this comment

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

LGTM thank you!

@seanshpark seanshpark merged commit 3bfcdc0 into Samsung:master Sep 12, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready for review It is ready to review. Please review it. SSAFY
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants