Skip to content

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

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