[GitHub] [incubator-tvm] comaniac commented on pull request #5689: [PatternLang] Add ConstantPattern
comaniac commented on pull request #5689: URL: https://github.com/apache/incubator-tvm/pull/5689#issuecomment-635675585 > @comaniac @mbrookhart good to go? Yeah I'm fine with that. 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: us...@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on pull request #5689: [PatternLang] Add ConstantPattern
comaniac commented on pull request #5689:
URL: https://github.com/apache/incubator-tvm/pull/5689#issuecomment-635672040
OK so here is a conclusion for another follow-up PR to deal with constant
lifting.
- Pattern input `ConstantPattern`:
- Only match constant nodes and keep them in the partitioned functions.
In this case, the arguments of partitioned functions will be reduced.
- Pattern input `VarPattern('x')`:
- Match only the var node with a specified name hint. In this case, the
arguments of partitioned function is fixed.
- Pattern input `wildcard` or `VarPattern('x') | ConstantPattern`:
- Match both constant and var node but life constant nodes. In this
case, the arguments of partitioned function is fixed whever it matches constant
or var.
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on pull request #5689: [PatternLang] Add ConstantPattern
comaniac commented on pull request #5689:
URL: https://github.com/apache/incubator-tvm/pull/5689#issuecomment-635667979
That's the behavior I just realized with above examples. In this case, what
would be the behavior if we specify `VarPattern('x') | ConstantPattern()`?
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on pull request #5689: [PatternLang] Add ConstantPattern
comaniac commented on pull request #5689:
URL: https://github.com/apache/incubator-tvm/pull/5689#issuecomment-635663626
OK I can finally reproduce this case:
```
fn (%x: Tensor[(1, 3, 224, 224), float32], %b: Tensor[(3), float32]) ->
Tensor[(1, 3, 222, 222), float32] {
%1 = fn (%x1: Tensor[(1, 3, 224, 224), float32], %b1: Tensor[(3),
float32], Composite="pat") -> Tensor[(1, 3, 222, 222), float32] {
%0 = nn.conv2d(%x1, meta[relay.Constant][0] /* ty=Tensor[(3, 3, 3, 3),
float64] */ /* ty=Tensor[(3, 3, 3, 3), float64] */, padding=[0, 0, 0, 0]) /*
ty=Tensor[(1, 3, 222, 222), float32] */;
nn.bias_add(%0, %b1) /* ty=Tensor[(1, 3, 222, 222), float32] */
};
%1(%x, %b) /* ty=Tensor[(1, 3, 222, 222), float32] */
}
// meta data omitted. you can use show_meta_data=True to include meta data
```
Combining wih the previous example we know that old `MergeComposite`
preserves constant node when the pattern is also a constant; otherwise it will
lift constant nodes. In the DNNL codegen, the patterns are all specified as
VarNode, so the case that matches a pattern with constant nodess never shows up
in unit tests and that's why we missed it.
However, I personally think this behavior is weird. Generally speaking,
whatever we specify `var` or `const` in the pattern, we may need or not need
constant lifting. It seems to me that this partition behavior should not be
bind with patterns but should be a partition option.
Anyways, I think the behavior of constant lifting should be discussed in
another topic and does not relate to this PR.
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on pull request #5689: [PatternLang] Add ConstantPattern
comaniac commented on pull request #5689:
URL: https://github.com/apache/incubator-tvm/pull/5689#issuecomment-635649921
I made an example and tested it with old `MergeComposite` pass (master
commit 6100112a150540588ddc9abb36dea0ff961f4301). It behaves as I expected. The
partitioned composite function still has 3 arguments even `%w` has been bind.
@mbaret could you double check?
```python
import tvm
from tvm import relay
from tvm import tir
from tvm.relay.testing import run_opt_pass
from tvm.relay.build_module import bind_params_by_name
import numpy as np
# Make a graph
x = relay.var('x', shape=(1, 3, 224, 224))
w = relay.var('w', shape=(3, 3, 3, 3))
b = relay.var('b', shape=(3,))
conv2d = relay.op.nn.conv2d(x, w)
out = relay.op.nn.bias_add(conv2d, b)
func = relay.Function([x, w, b], out)
mod = tvm.IRModule.from_expr(func)
mod["main"] = bind_params_by_name(mod["main"],
{'w': tvm.nd.array(np.ones(shape=(3, 3, 3,
3)))})
print('=== Before ===')
print(mod['main'].body)
def pat():
x = relay.var('x', shape=(1, 3, 224, 224))
w = relay.var('w', shape=(3, 3, 3, 3))
b = relay.var('b', shape=(3,))
conv2d = relay.op.nn.conv2d(x, w)
out = relay.op.nn.bias_add(conv2d, b)
return out
pattern_table = [('pat', pat())]
result = run_opt_pass(mod['main'],
relay.transform.MergeComposite(pattern_table))
print('=== After ===')
print(result)
```
```
=== Before ===
free_var %x: Tensor[(1, 3, 224, 224), float32]
%0 = nn.conv2d(%x, meta[relay.Constant][0] /* ty=Tensor[(3, 3, 3, 3),
float64] */ /* ty=Tensor[(3, 3, 3, 3), float64] */, padding=[0, 0, 0, 0]) /*
ty=Tensor[(1, 3, 222, 222), float32] */;
free_var %b: Tensor[(3), float32]
nn.bias_add(%0, %b) /* ty=Tensor[(1, 3, 222, 222), float32] */
// meta data omitted. you can use show_meta_data=True to include meta data
=== After ===
fn (%x: Tensor[(1, 3, 224, 224), float32], %b: Tensor[(3), float32]) ->
Tensor[(1, 3, 222, 222), float32] {
%1 = fn (%x1: Tensor[(1, 3, 224, 224), float32], %w: Tensor[(3, 3, 3, 3),
float64], %b1: Tensor[(3), float32], Composite="pat") -> Tensor[(1, 3, 222,
222), float32] {
%0 = nn.conv2d(%x1, %w, padding=[0, 0, 0, 0]) /* ty=Tensor[(1, 3, 222,
222), float32] */;
nn.bias_add(%0, %b1) /* ty=Tensor[(1, 3, 222, 222), float32] */
};
%1(%x, meta[relay.Constant][0] /* ty=Tensor[(3, 3, 3, 3), float64] */ /*
ty=Tensor[(3, 3, 3, 3), float64] */, %b) /* ty=Tensor[(1, 3, 222, 222),
float32] */
}
// meta data omitted. you can use show_meta_data=True to include meta data
```
@masahi I just checked with this PR. The unit test hits the line you pointed
out twice.
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on pull request #5689: [PatternLang] Add ConstantPattern
comaniac commented on pull request #5689: URL: https://github.com/apache/incubator-tvm/pull/5689#issuecomment-635637903 > Thanks for the PR so soon! Is there an example of how partition works on a constant match? In particular, does the constant remain propagated into the body? An example can be found in the unit test: https://github.com/apache/incubator-tvm/pull/5689/files#diff-f9920485e5e341787129348ce1985db9R213 Also, could you provide an example to show your expection of constant propogation? > > On a general point, it's helpful if reformats are kept separate from feature additions for reviews so it's easy to see what the changes are. Yeah the bugfix should be put to a separate PR for sure. For re-formatting, I didn't intentionally reformat the unit test file. VSCode by default auto-formats the file once I paste some code. I was even considering to revert auto-formatting due to the reason you pointed out. However, I checked the re-formatting and it is mostly just blank and too-long lines, so I think it should be fine for this small feature PR. 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: us...@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on pull request #5689: [PatternLang] Add ConstantPattern
comaniac commented on pull request #5689: URL: https://github.com/apache/incubator-tvm/pull/5689#issuecomment-635629161 > To make other syntactic sugar, you could add an is_const() that simiply creats a new ConstPattern, but I don't think it's strictly necessary. I could do that. How about `is_tuple` and `is_tuple_get_item`? > For my own edification, what did you use to auto-format the test file? I tried autopep8 on it a while back and got really bad results, so I ignored it after that (as does the linter). I used `yapf` with a self [.style.yapf](https://gist.github.com/comaniac/388921955dbe6c13561c4734aaa7a3e3) I put under the TVM home. 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: us...@infra.apache.org
