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

Sns transpose support #56

Conversation

IvanNovoselov
Copy link
Collaborator

Details:

  • This is a copy of PR 13870 please refer to the original PR for the details

@a-sidorova a-sidorova mentioned this pull request Nov 24, 2022
3 tasks
@IvanNovoselov IvanNovoselov force-pushed the sns_transpose_support branch 2 times, most recently from a153865 to 95739fc Compare November 29, 2022 14:04
Copy link
Collaborator

@dmitry-gorokhov dmitry-gorokhov left a comment

Choose a reason for hiding this comment

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

First part

Copy link
Collaborator

@dmitry-gorokhov dmitry-gorokhov left a comment

Choose a reason for hiding this comment

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

rest

Copy link
Collaborator

@dmitry-gorokhov dmitry-gorokhov left a comment

Choose a reason for hiding this comment

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

As agreed all the comments will be resolved in separate PR

@dmitry-gorokhov dmitry-gorokhov merged commit 5d3e6f4 into a-sidorova:feature/snippets/mha Dec 1, 2022
Comment on lines +31 to +34
const auto transpose = ov::as_type_ptr<ngraph::opset1::Transpose>(
pattern_to_output.at(match_transpose).get_node_shared_ptr());

const auto order = ov::as_type_ptr<ov::op::v0::Constant>(pattern_to_output.at(match_order).get_node_shared_ptr());
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid some potential warnings from static analyzers I would recommend to check transpose and order on nullptr

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's an interesting question. Do we have a common practice on this point?
Because I intentionally skipped the checks to avoid extra penalty in runtime? since the matcher guarantees that they are not nullptr. Let's discuss this on the sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants