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][Relay] skip leaf args when matching 'path' part for dominator pattern #16983

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

wanghuibin0
Copy link
Contributor

@wanghuibin0 wanghuibin0 commented May 9, 2024

This is a patch for PR #15473
That PR tried to solve the issues described in RFC, but introduced another issue, probably unintentionally.
The new issue occurs, when there are some leaf args of some OPs along the path during matching 'path' part for dominator pattern. For example, suppose we have a dominator pattern as follows:

    conv2d_pat = is_op("nn.conv2d")(wildcard(), wildcard())
    eltwise_pat = (wildcard().has_attr({"TOpPattern": K_ELEMWISE}))(None)
    broadcast_pat = (wildcard().has_attr({"TOpPattern": K_BROADCAST}))(None)
    path_pat = eltwise_pat | broadcast_pat
    injective_pat = (wildcard().has_attr({"TOpPattern": K_INJECTIVE}))(wildcard())
    pattern = DominatorPattern(conv2d_pat, path_pat, injective_pat)

and we want to match the graph:

input  weight
  \     /
   \   /
   conv2d  bias
     \     /
      \   /
    bias_add
        |
      relu
        |
     reshape

The pattern should match this graph since conv2d dominates reshape along bias_add+relu, provided that we accept the original definition of "dominator" in TVM. But the change in PR #15473 makes the matching fail because it treats bias as an addtional path.

This is fixed by skipping the leaf arguments when matching the 'path' part of the dominator pattern.

The issue may be rooted in some confusion of the definition of "dominator" in TVM, as described in this post. But at present, since the original definition of "dominator" remains unchanged in the real codebase, I think my patch may still make sense.

@wanghuibin0 wanghuibin0 force-pushed the dominatorpattern branch 3 times, most recently from 399bcb3 to 2bfdb17 Compare May 13, 2024 02:46
@wanghuibin0 wanghuibin0 changed the title bugfix: skip costant arg when matching path for dominator pattern [BugFix][Relay] skip leaf args when matching 'path' part for dominator pattern May 13, 2024
@wanghuibin0
Copy link
Contributor Author

wanghuibin0 commented May 13, 2024

Some changes are made to fix an issue of matching dominator pattern. Could you help review this? cc @wrongtest-intellif @kfeng123

@wanghuibin0 wanghuibin0 marked this pull request as ready for review May 13, 2024 04:08
Copy link
Contributor

@wrongtest-intellif wrongtest-intellif left a comment

Choose a reason for hiding this comment

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

LGTM

@wrongtest-intellif wrongtest-intellif merged commit e6bfaf8 into apache:main Jun 21, 2024
16 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