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

[PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs #7754

Merged
merged 4 commits into from
Apr 3, 2021

Conversation

mbrookhart
Copy link
Contributor

Copy link
Contributor

@ekalda ekalda left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for doing this! :)

src/relay/ir/dataflow_matcher.cc Outdated Show resolved Hide resolved
x = relay.var("x")
y = relay.var("y")
z = x + y
tuple_pattern = is_tuple(None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the use of None? What would happen if the brackets were left empty (similarly to is_constant())? (Just asking for enlightenment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to keep the API consistent between tuple, call, and function. There are many places were we may have functions or ops with no arguments, so calling an op with op() yields an empty input list, which is a concrete pattern we want to keep matching. I choose None to easily identify the fuzzy case against the empty input case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree the use of None here is a bit confusing, I think I'd naturally interpret it the other way around actually. Specifically, None seems like you're explicitly requesting that there be no arguments rather than an unknown number. Just as a suggestion, would a syntax like is_tuple(args()) be clearer where we explicitly introduce a new pattern which matches a variable number of arguments?

Copy link
Contributor Author

@mbrookhart mbrookhart Mar 30, 2021

Choose a reason for hiding this comment

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

The problem is that we're not actually talking about a pattern, which explicitly represents an AST node. Instead, we're trying to represent an attribute of an AST node, particularly an attribute of the type Array<Expr>, that's what needs to be passed in. In the backend I need to be able to distinguish between a valid array of size 0 that I want to match on, and an invalid fuzzy array I don't want to match on. The simplest way to do that is to store a nullptr, and when I pass python None to the backend, that's what I get.

I could introduce something like an arg+ pattern that goes into the argument list, but that screws with the locality of the pattern matcher and creates a need to do a lot of look-ahead to determine how to match arguments so I don't accidentally index into an array I shouldn't or reject the match because the arguments don't match. It's feasible, but messy.

Copy link
Member

Choose a reason for hiding this comment

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

@ekalda @mbaret do we agree with the interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with it, I think all alternatives to None are non-obvious in their own way. Maybe worth adding an example to https://tvm.apache.org/docs/langref/relay_pattern.html# so that people would be aware of this functionality and the interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @ekalda, in the absence of a more self-documenting interface explicit documentation is the next best option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense, thanks! I'll add this to the docs (fix my spelling mistake :D) And re-push.

@masahi masahi self-assigned this Mar 30, 2021
@masahi masahi merged commit 7e68b4d into apache:main Apr 3, 2021
@masahi
Copy link
Member

masahi commented Apr 3, 2021

thanks @mbrookhart @ekalda @mbaret

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
…ith variable numbers of inputs (apache#7754)

* Allow TuplePattern to have null fields and match any tuple

* support matching functions and call nodes with variable numbers of parameters

* remove development code that was commented out

* add docs for fuzzy matching
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
…ith variable numbers of inputs (apache#7754)

* Allow TuplePattern to have null fields and match any tuple

* support matching functions and call nodes with variable numbers of parameters

* remove development code that was commented out

* add docs for fuzzy matching
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
…ith variable numbers of inputs (apache#7754)

* Allow TuplePattern to have null fields and match any tuple

* support matching functions and call nodes with variable numbers of parameters

* remove development code that was commented out

* add docs for fuzzy matching
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request May 11, 2021
…ith variable numbers of inputs (apache#7754)

* Allow TuplePattern to have null fields and match any tuple

* support matching functions and call nodes with variable numbers of parameters

* remove development code that was commented out

* add docs for fuzzy matching
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.

4 participants