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/pass] Refactor FuseAddWithTConvPass #13745

Merged

Conversation

jiwaszki
Copy link
Contributor

This commit changes the order of searching for the pattern and adds tests for the pass.

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

@jiwaszki
Copy link
Contributor Author

For: #13685

@seanshpark some of my findings:

  • As we discussed the checking of only one successor in [luci/pass] Introduce FuseMulWithFullyConnectedPass #13607 (comment) - it looks like the practice emerges in other passes as well.
    Here are the original lines from fuse_add_with_tconv:
    // get add node
    auto tconv_output = loco::succs(tconv);
    assert(tconv_output.size() == 1);
    auto add = dynamic_cast<luci::CircleAdd *>(*tconv_output.begin());

    I have decided not to change this check as the task is to refactor order of the search.
  • There were no tests in FuseAddWithTConvPass.test.cpp besides name checking, so I have added them.

This commit changes the order of searching for the pattern and adds tests for the pass.

ONE-DCO-1.0-Signed-off-by: Jan Iwaszkiewicz <[email protected]>
@jiwaszki jiwaszki force-pushed the jiwaszki/fix_order_fuse_add_tconv_pass branch from 2aa80ac to fdd1efe Compare August 22, 2024 12:59
@jiwaszki jiwaszki changed the title [DRAFT][luci/pass] Refactor FuseAddWithTConvPass [luci/pass] Refactor FuseAddWithTConvPass Aug 28, 2024
@jiwaszki
Copy link
Contributor Author

@seanshpark @jinevening PR is ready to review. I cannot add you to reviewers or change the label (is it blocked for contributors?).

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020 Samsung Electronics Co., Ltd. All Rights Reserved
* Copyright (c) 2020-2024 Samsung Electronics Co., Ltd. All Rights Reserved
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to do this change

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 wasn't sure about it as well... now I have guidance to follow. Will do!

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2021 Samsung Electronics Co., Ltd. All Rights Reserved
* Copyright (c) 2021-2024 Samsung Electronics Co., Ltd. All Rights Reserved
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@jiwaszki jiwaszki marked this pull request as ready for review August 28, 2024 09:44
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 543d655 into Samsung:master Aug 28, 2024
11 checks passed
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.

2 participants