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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

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
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.

None yet

1 participant