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 2020/06/12 18:49:12 UTC
[GitHub] [incubator-tvm] t-vi opened a new pull request #5791: Add a combine batch_matmul pass
t-vi opened a new pull request #5791:
URL: https://github.com/apache/incubator-tvm/pull/5791
Contrary what you might expect, this doesn't share as much code with the combine dense pass as it does with the combine 2d conv pass. This is because it concatenates the "output feature" dimensions just like the 2d conv pass concatenates output channels, whereas combine dense stacks the various matmul arguments.
I'm not sure if there is a deeper reason not to concatenate for dense, too, but maybe there is.
----------------------------------------------------------------
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] [incubator-tvm] t-vi commented on pull request #5791: Add a combine batch_matmul pass
Posted by GitBox <gi...@apache.org>.
t-vi commented on pull request #5791:
URL: https://github.com/apache/incubator-tvm/pull/5791#issuecomment-644258600
@mbrookhart Yes, my use-case is transformers. The PyTorch frontend translates the matmul used in HuggingFace `transformer`'s BERT into `batch_matmul`. The speedup is 1.5x-2x-ish on ROCm (gfx906) and also some on a GTX1080Ti even though it currently hits a reshape right after `batch_matmul`. I don't quite reach the speed of ONNXRuntime yet.
I'm currently preparing a detailed writeup (and that's the pattern of my recent PRs - tuneable BMM, this, support for integers and other non-float32 PyTorch frontend).
I imagine that it would be cool to move the pass to a pattern-matching. I would expect that it would replace the code shared by the combine passes of `batch_matmul` and `conv2d` (and to some extend the `dense` combiner rather than the part that's separate. I have been wondering about the efficiency of `dense` btw. - it mentions BERT as a use-case in the code comments but it is unclear to me whether the `dense` -> `batch_matmul` with "duplicated" (possibly stride 0) input is better than `dense` -> `dense` with non-contiguous results (though the columns would still be and only the rows would be interleaved between the ops). But then I haven't looked a lot at how TVM deals with strides (which is relatively significant because the self-attention typically has some reshapes that would be nice to fuse).
----------------------------------------------------------------
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] [incubator-tvm] t-vi closed pull request #5791: Add a combine batch_matmul pass
Posted by GitBox <gi...@apache.org>.
t-vi closed pull request #5791:
URL: https://github.com/apache/incubator-tvm/pull/5791
----------------------------------------------------------------
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] [incubator-tvm] tqchen commented on pull request #5791: Add a combine batch_matmul pass
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #5791:
URL: https://github.com/apache/incubator-tvm/pull/5791#issuecomment-645112888
cc @vinx13 please also help to take a look
----------------------------------------------------------------
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] [incubator-tvm] t-vi commented on a change in pull request #5791: Add a combine batch_matmul pass
Posted by GitBox <gi...@apache.org>.
t-vi commented on a change in pull request #5791:
URL: https://github.com/apache/incubator-tvm/pull/5791#discussion_r441417435
##########
File path: python/tvm/relay/transform/transform.py
##########
@@ -307,6 +308,37 @@ def CombineParallelDense(min_num_branches=3):
"""
return _ffi_api.CombineParallelDense(min_num_branches)
+def CombineParallelBatchMatmul(min_num_branches=3):
+ """Combine multiple dense operators into one. For example:
+
+ .. code-block
+ data
+ / \
+ dense (2,2) dense (2,2)
Review comment:
Fixed both, thank you for catching!
----------------------------------------------------------------
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] [incubator-tvm] t-vi commented on a change in pull request #5791: Add a combine batch_matmul pass
Posted by GitBox <gi...@apache.org>.
t-vi commented on a change in pull request #5791:
URL: https://github.com/apache/incubator-tvm/pull/5791#discussion_r441386661
##########
File path: python/tvm/relay/transform/transform.py
##########
@@ -307,6 +308,37 @@ def CombineParallelDense(min_num_branches=3):
"""
return _ffi_api.CombineParallelDense(min_num_branches)
+def CombineParallelBatchMatmul(min_num_branches=3):
+ """Combine multiple dense operators into one. For example:
Review comment:
Oha, caught me copy pasting... Thank you!
----------------------------------------------------------------
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] [incubator-tvm] comaniac commented on pull request #5791: Add a combine batch_matmul pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #5791:
URL: https://github.com/apache/incubator-tvm/pull/5791#issuecomment-643887744
This might be off the topic. I think it's possible to use the pattern language to replace all "combine parallel XX op" passes. Maybe we could create an issue to check it.
cc @mbrookhart @tqchen
----------------------------------------------------------------
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] [incubator-tvm] mbrookhart commented on pull request #5791: Add a combine batch_matmul pass
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #5791:
URL: https://github.com/apache/incubator-tvm/pull/5791#issuecomment-644241874
@t-vi Interesting PR! Thank you for submitting it! I presume the use case for this is in Transformer-like models? Do you see a perf benefit from the rewrite?
> This might be off the topic. I think it's possible to use the pattern language to replace all "combine parallel XX op" passes. Maybe we could create an issue to check it.
>
> cc @mbrookhart @tqchen
I would imagine yes, it would be pretty easy to implement this as a pattern and a rewrite. A pattern solution might have some complication though, I don't think we currently have a way to match something with 2 or 3 or 4 branches in the same pattern, it would require a number of patterns. I'll think about 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:
users@infra.apache.org
[GitHub] [incubator-tvm] vinx13 merged pull request #5791: Add a combine batch_matmul pass
Posted by GitBox <gi...@apache.org>.
vinx13 merged pull request #5791:
URL: https://github.com/apache/incubator-tvm/pull/5791
----------------------------------------------------------------
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] [incubator-tvm] vinx13 commented on a change in pull request #5791: Add a combine batch_matmul pass
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #5791:
URL: https://github.com/apache/incubator-tvm/pull/5791#discussion_r441347820
##########
File path: python/tvm/relay/transform/transform.py
##########
@@ -307,6 +308,37 @@ def CombineParallelDense(min_num_branches=3):
"""
return _ffi_api.CombineParallelDense(min_num_branches)
+def CombineParallelBatchMatmul(min_num_branches=3):
+ """Combine multiple dense operators into one. For example:
Review comment:
Combine multiple batch_matmul
##########
File path: python/tvm/relay/transform/transform.py
##########
@@ -307,6 +308,37 @@ def CombineParallelDense(min_num_branches=3):
"""
return _ffi_api.CombineParallelDense(min_num_branches)
+def CombineParallelBatchMatmul(min_num_branches=3):
+ """Combine multiple dense operators into one. For example:
+
+ .. code-block
+ data
+ / \
+ dense (2,2) dense (2,2)
Review comment:
need to be updated?
----------------------------------------------------------------
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