You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/03/26 21:47:21 UTC
[GitHub] [tvm] mbrookhart opened a new pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
mbrookhart opened a new pull request #7754:
URL: https://github.com/apache/tvm/pull/7754
Also fixes partitioning functions whose bodies are matched fuzzily.
cc @jroesch @masahi @slyubomirsky @ekalda
Also see discuss forums here:
https://discuss.tvm.apache.org/t/tuplepattern-for-any-number-of-inputs-to-the-tuple/8926
https://discuss.tvm.apache.org/t/pattern-language-matching-a-tuple-with-variable-size/9497
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on a change in pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #7754:
URL: https://github.com/apache/tvm/pull/7754#discussion_r604230272
##########
File path: tests/python/relay/test_dataflow_pattern.py
##########
@@ -1375,6 +1392,63 @@ def test_partition_overused():
assert pattern.partition(out) == out
+def test_partition_fuzzy_tuple():
+ x = relay.var("x")
+ y = relay.var("y")
+ z = x + y
+ tuple_pattern = is_tuple(None)
Review comment:
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\<Exp\r>, 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.
##########
File path: tests/python/relay/test_dataflow_pattern.py
##########
@@ -1375,6 +1392,63 @@ def test_partition_overused():
assert pattern.partition(out) == out
+def test_partition_fuzzy_tuple():
+ x = relay.var("x")
+ y = relay.var("y")
+ z = x + y
+ tuple_pattern = is_tuple(None)
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbaret commented on a change in pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #7754:
URL: https://github.com/apache/tvm/pull/7754#discussion_r605482977
##########
File path: tests/python/relay/test_dataflow_pattern.py
##########
@@ -1375,6 +1392,63 @@ def test_partition_overused():
assert pattern.partition(out) == out
+def test_partition_fuzzy_tuple():
+ x = relay.var("x")
+ y = relay.var("y")
+ z = x + y
+ tuple_pattern = is_tuple(None)
Review comment:
I agree with @ekalda, in the absence of a more self-documenting interface explicit documentation is the next best option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on a change in pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #7754:
URL: https://github.com/apache/tvm/pull/7754#discussion_r604210239
##########
File path: tests/python/relay/test_dataflow_pattern.py
##########
@@ -1375,6 +1392,63 @@ def test_partition_overused():
assert pattern.partition(out) == out
+def test_partition_fuzzy_tuple():
+ x = relay.var("x")
+ y = relay.var("y")
+ z = x + y
+ tuple_pattern = is_tuple(None)
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on a change in pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
Posted by GitBox <gi...@apache.org>.
ekalda commented on a change in pull request #7754:
URL: https://github.com/apache/tvm/pull/7754#discussion_r605478688
##########
File path: tests/python/relay/test_dataflow_pattern.py
##########
@@ -1375,6 +1392,63 @@ def test_partition_overused():
assert pattern.partition(out) == out
+def test_partition_fuzzy_tuple():
+ x = relay.var("x")
+ y = relay.var("y")
+ z = x + y
+ tuple_pattern = is_tuple(None)
Review comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7754:
URL: https://github.com/apache/tvm/pull/7754#discussion_r605399400
##########
File path: src/relay/ir/dataflow_matcher.cc
##########
@@ -898,6 +932,11 @@ class PatternPartitioner : protected MixedModeMutator {
public:
Expr Partition(const DFPattern& pattern, const Expr& pre, const Map<String, ObjectRef>& attrs,
PackedFunc check) {
+ if (pattern.as<FunctionPatternNode>()) {
+ LOG(WARNING) << "Paritioning a Function that isn't called doesn't make sense, skipping"
Review comment:
@mbrookhart
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7754:
URL: https://github.com/apache/tvm/pull/7754#issuecomment-812838540
thanks @mbrookhart @ekalda @mbaret
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7754:
URL: https://github.com/apache/tvm/pull/7754#discussion_r605399695
##########
File path: tests/python/relay/test_dataflow_pattern.py
##########
@@ -1375,6 +1392,63 @@ def test_partition_overused():
assert pattern.partition(out) == out
+def test_partition_fuzzy_tuple():
+ x = relay.var("x")
+ y = relay.var("y")
+ z = x + y
+ tuple_pattern = is_tuple(None)
Review comment:
@ekalda @mbaret do we agree with the interface?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on a change in pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
Posted by GitBox <gi...@apache.org>.
ekalda commented on a change in pull request #7754:
URL: https://github.com/apache/tvm/pull/7754#discussion_r604006798
##########
File path: src/relay/ir/dataflow_matcher.cc
##########
@@ -898,6 +932,11 @@ class PatternPartitioner : protected MixedModeMutator {
public:
Expr Partition(const DFPattern& pattern, const Expr& pre, const Map<String, ObjectRef>& attrs,
PackedFunc check) {
+ if (pattern.as<FunctionPatternNode>()) {
+ LOG(WARNING) << "Paritioning a Function that isn't called doesn't make sense, skipping"
Review comment:
nit - Partitioning
##########
File path: tests/python/relay/test_dataflow_pattern.py
##########
@@ -1375,6 +1392,63 @@ def test_partition_overused():
assert pattern.partition(out) == out
+def test_partition_fuzzy_tuple():
+ x = relay.var("x")
+ y = relay.var("y")
+ z = x + y
+ tuple_pattern = is_tuple(None)
Review comment:
Why the use of None? What would happen if the brackets were left empty (similarly to is_constant())? (Just asking for enlightenment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on a change in pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #7754:
URL: https://github.com/apache/tvm/pull/7754#discussion_r604230272
##########
File path: tests/python/relay/test_dataflow_pattern.py
##########
@@ -1375,6 +1392,63 @@ def test_partition_overused():
assert pattern.partition(out) == out
+def test_partition_fuzzy_tuple():
+ x = relay.var("x")
+ y = relay.var("y")
+ z = x + y
+ tuple_pattern = is_tuple(None)
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbaret commented on a change in pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #7754:
URL: https://github.com/apache/tvm/pull/7754#discussion_r604223799
##########
File path: tests/python/relay/test_dataflow_pattern.py
##########
@@ -1375,6 +1392,63 @@ def test_partition_overused():
assert pattern.partition(out) == out
+def test_partition_fuzzy_tuple():
+ x = relay.var("x")
+ y = relay.var("y")
+ z = x + y
+ tuple_pattern = is_tuple(None)
Review comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on a change in pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #7754:
URL: https://github.com/apache/tvm/pull/7754#discussion_r605718681
##########
File path: tests/python/relay/test_dataflow_pattern.py
##########
@@ -1375,6 +1392,63 @@ def test_partition_overused():
assert pattern.partition(out) == out
+def test_partition_fuzzy_tuple():
+ x = relay.var("x")
+ y = relay.var("y")
+ z = x + y
+ tuple_pattern = is_tuple(None)
Review comment:
This makes sense, thanks! I'll add this to the docs (fix my spelling mistake :D) And re-push.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi merged pull request #7754: [PatternMatcher] Support matching tuples, call nodes, and functions with variable numbers of inputs
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #7754:
URL: https://github.com/apache/tvm/pull/7754
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org