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

[BugFix][Relax] change FuseOpsByPattern strategy to pattern-match maximal subgraph #16922

Merged
merged 2 commits into from
May 10, 2024

Conversation

wanghuibin0
Copy link
Contributor

@wanghuibin0 wanghuibin0 commented Apr 25, 2024

At first, FuseOpsByPattern for Relax used a strategy to match maximal subgraph.
But this was changed in a previous PR #15308.
The intention was to fix a bug: in FuseOpsByPattern, when two matched subgraph are intersected, they are incorrectly merged together.
The bug was fixed by returning only the first subgraph if multiple subgraph are intersected.
The solution successfully solved the problem described there, but unfortunately, resulted in a minimal subgraph match strategy, probably unexpected.

Let me clarify with a compute graph as an example:

lv0 = R.matmul(x, y)
lv1 = R.add(lv0, bias)
lv2 = R.clip(lv1, -128, 127)

Consider this pattern: matmul->add | matmul->add->clip. It should match both lv0+lv1 and lv0+lv1+lv2. The solution in the aforementioned PR matches only lv0+lv1. But a stategy to match all lv0+lv1+lv2 is expected.

This bug has been fixed by checking, when two matched subgraph are intersected, whether they have an inclusion relationship(i.e. whether subgraph A is contained in subgraph B), and if they have, the larger one is retained.

cc @wrongtest-intellif

@wanghuibin0 wanghuibin0 force-pushed the fuseopsbypattern branch 7 times, most recently from f1fa32a to 9d46d67 Compare May 9, 2024 11:54
@wanghuibin0 wanghuibin0 changed the title change FuseOpsByPattern policy to match max subgraph [BugFix][Relax] change FuseOpsByPattern strategy to pattern-match maximal subgraph May 10, 2024
@wanghuibin0 wanghuibin0 marked this pull request as ready for review May 10, 2024 09:12
@wrongtest-intellif
Copy link
Contributor

hi, could you kindly take a look at this change? I understand it is one more patch to #15308. thanks! @masahi

Copy link
Member

@masahi masahi left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks!

@wrongtest-intellif wrongtest-intellif merged commit 2565aa3 into apache:main May 10, 2024
25 checks passed
@wanghuibin0 wanghuibin0 deleted the fuseopsbypattern branch May 14, 2024 02:48
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.

3 participants