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

[PatternLang] Don't rewrite expressions used outside of the pattern #5930

Merged
merged 2 commits into from
Jun 26, 2020

Conversation

mbrookhart
Copy link
Contributor

@comaniac

While investigating #5928, I discovered an issue where the pattern language would rewrite expressions used outside of the pattern, this prevents that issue, but doesn't fix #5928

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

The changes LGTM. Leave some comments to make sure I understand the semantic correctly.

T2 = BN[0]
add = T1 + T2

assert tuple_get_item_node.partition(add) == add
Copy link
Contributor

Choose a reason for hiding this comment

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

Two questions to this test case:

  1. Should we use structural_equal here?
  2. Does that mean we do not even treat BN -> T2 as a match anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partitioning either path is invalid, since either side would need intermediate nodes from the other, so we expect the original expression to come back unchanged, thus the ==. We treat it as a match, but we don't treat it as something we can independently rewrite.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. That makes sense.

relu = relay.op.nn.relu(conv2d)
out = relu + conv2d

assert pattern.partition(out) == out
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, fusing the conv and relu would make the rest of the expr invalid, so we expect the expr to come back unchanged.

@tqchen tqchen merged commit e1a1c2a into apache:master Jun 26, 2020
@tqchen
Copy link
Member

tqchen commented Jun 26, 2020

Thanks @mbrookhart , Thanks @comaniac for reviewing

@mbrookhart mbrookhart deleted the mbrookhart/overused_patterns branch June 26, 2020 15:28
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 30, 2020
…pache#5930)

* Don't rewrite expressions used outside of the pattern

* add comments
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Jul 2, 2020
…pache#5930)

* Don't rewrite expressions used outside of the pattern

* add comments
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.

[PatternLang] The pattern failed to match some subgraphs in a model
3 participants