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/07/30 18:00:43 UTC
[GitHub] [tvm] ymwangg opened a new pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
ymwangg opened a new pull request #8607:
URL: https://github.com/apache/tvm/pull/8607
This PR added transpose_a/b for TensorRT batch_matmul, fixed a warning and compilation error with TensorRT-8. It also removed the redundant transpose op in onnx matmul.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ymwangg commented on pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
Posted by GitBox <gi...@apache.org>.
ymwangg commented on pull request #8607:
URL: https://github.com/apache/tvm/pull/8607#issuecomment-893063069
@trevor-m @jcf94 Please review again when you get a chance.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] trevor-m merged pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
Posted by GitBox <gi...@apache.org>.
trevor-m merged pull request #8607:
URL: https://github.com/apache/tvm/pull/8607
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ymwangg commented on a change in pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
Posted by GitBox <gi...@apache.org>.
ymwangg commented on a change in pull request #8607:
URL: https://github.com/apache/tvm/pull/8607#discussion_r681275178
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -770,10 +770,8 @@ def flatten_to_nd(x, x_shape, nd=3):
# Convert a and b into 3 dimensional tensors.
a = flatten_to_nd(inputs[0], a_shape, 3)
b = flatten_to_nd(inputs[1], b_shape, 3)
- # Transpose matrix dimensions of b.
- b = _op.transpose(b, [0, 2, 1])
# Perform a batch matmul.
- output = _op.nn.batch_matmul(a, b)
+ output = _op.nn.batch_matmul(a, b, transpose_b=False)
Review comment:
@jcf94 Thanks for the pointer. I will refactor to make NN optional.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jcf94 commented on a change in pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
Posted by GitBox <gi...@apache.org>.
jcf94 commented on a change in pull request #8607:
URL: https://github.com/apache/tvm/pull/8607#discussion_r680612911
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -770,10 +770,8 @@ def flatten_to_nd(x, x_shape, nd=3):
# Convert a and b into 3 dimensional tensors.
a = flatten_to_nd(inputs[0], a_shape, 3)
b = flatten_to_nd(inputs[1], b_shape, 3)
- # Transpose matrix dimensions of b.
- b = _op.transpose(b, [0, 2, 1])
# Perform a batch matmul.
- output = _op.nn.batch_matmul(a, b)
+ output = _op.nn.batch_matmul(a, b, transpose_b=False)
Review comment:
Thanks! @ymwangg
Just a little concern about changing the default behavior of framework frontend, since currently the default topi schedule support for NN format is not as strong as the original NT one.
This may cause confusions to those who have used onnx frontend before or who is using onnx frontend now.
To give an example, I've added an extra config to TensorFlow frontend which uses the NT format by default but provides an option to use the normal format. I think that would be better before we have prepared a strong enough topi.
p.s.: You see, I've also kept the default layout for `nn.batch_matmul` to be the original NT.
cc @comaniac
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ymwangg commented on a change in pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
Posted by GitBox <gi...@apache.org>.
ymwangg commented on a change in pull request #8607:
URL: https://github.com/apache/tvm/pull/8607#discussion_r680189423
##########
File path: src/runtime/contrib/tensorrt/tensorrt_logger.h
##########
@@ -39,7 +39,7 @@ class TensorRTLogger : public nvinfer1::ILogger {
public:
TensorRTLogger() : TensorRTLogger(Severity::kWARNING) {}
explicit TensorRTLogger(Severity severity) : reportable_severity(severity) {}
- void log(Severity severity, const char* msg) override {
+ void log(Severity severity, const char* msg) noexcept override {
Review comment:
Yes, this is required for TRT8. I've tested TRT7 and it works. In TRT 8 the signature changes to`virtual void log(Severity severity, AsciiChar const* msg) noexcept = 0;` from `virtual void log(Severity severity, const char* msg) TRTNOEXCEPT = 0;` in TRT7.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ymwangg commented on a change in pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
Posted by GitBox <gi...@apache.org>.
ymwangg commented on a change in pull request #8607:
URL: https://github.com/apache/tvm/pull/8607#discussion_r680190290
##########
File path: tests/python/contrib/test_tensorrt.py
##########
@@ -474,14 +474,22 @@ def get_graph(x_shape=(1, 16), k_shape=(32, 16)):
def test_batch_matmul():
- def get_graph(x_shape=(12, 128, 64), y_shape=(12, 128, 64)):
+ def get_graph(x_shape=(12, 128, 64), y_shape=(12, 64, 128), transa=False, transb=True):
x = relay.var("x", shape=(x_shape), dtype="float32")
y = relay.var("y", shape=(y_shape), dtype="float32")
- out = relay.nn.batch_matmul(x, y)
+ out = relay.nn.batch_matmul(
+ relay.transpose(x, [0, 2, 1]) if transa else x,
Review comment:
Good point, I've changed to using x/y_shape instead.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] trevor-m commented on a change in pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #8607:
URL: https://github.com/apache/tvm/pull/8607#discussion_r680147863
##########
File path: src/runtime/contrib/tensorrt/tensorrt_logger.h
##########
@@ -39,7 +39,7 @@ class TensorRTLogger : public nvinfer1::ILogger {
public:
TensorRTLogger() : TensorRTLogger(Severity::kWARNING) {}
explicit TensorRTLogger(Severity severity) : reportable_severity(severity) {}
- void log(Severity severity, const char* msg) override {
+ void log(Severity severity, const char* msg) noexcept override {
Review comment:
Is this change required to build against TRT 8? Could you check we can still compile against earlier TRT versions such as 7 even with this change? If we can't, then we should add `#if` around it.
##########
File path: tests/python/contrib/test_tensorrt.py
##########
@@ -474,14 +474,22 @@ def get_graph(x_shape=(1, 16), k_shape=(32, 16)):
def test_batch_matmul():
- def get_graph(x_shape=(12, 128, 64), y_shape=(12, 128, 64)):
+ def get_graph(x_shape=(12, 128, 64), y_shape=(12, 64, 128), transa=False, transb=True):
x = relay.var("x", shape=(x_shape), dtype="float32")
y = relay.var("y", shape=(y_shape), dtype="float32")
- out = relay.nn.batch_matmul(x, y)
+ out = relay.nn.batch_matmul(
+ relay.transpose(x, [0, 2, 1]) if transa else x,
Review comment:
I don't think you need these `relay.transpose` on the inputs to test functionality of transa/transb args.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jcf94 commented on a change in pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
Posted by GitBox <gi...@apache.org>.
jcf94 commented on a change in pull request #8607:
URL: https://github.com/apache/tvm/pull/8607#discussion_r680612911
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -770,10 +770,8 @@ def flatten_to_nd(x, x_shape, nd=3):
# Convert a and b into 3 dimensional tensors.
a = flatten_to_nd(inputs[0], a_shape, 3)
b = flatten_to_nd(inputs[1], b_shape, 3)
- # Transpose matrix dimensions of b.
- b = _op.transpose(b, [0, 2, 1])
# Perform a batch matmul.
- output = _op.nn.batch_matmul(a, b)
+ output = _op.nn.batch_matmul(a, b, transpose_b=False)
Review comment:
Just a little concern about changing the default behavior of framework frontend since currently the default topi schedule support for NN format is not as strong as the original NT one.
This may cause confusions to those who have used onnx frontend before.
To give an example, I've added an extra config to TensorFlow frontend which uses the NT format by default but provides a option to use normal format.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jcf94 commented on a change in pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
Posted by GitBox <gi...@apache.org>.
jcf94 commented on a change in pull request #8607:
URL: https://github.com/apache/tvm/pull/8607#discussion_r680612911
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -770,10 +770,8 @@ def flatten_to_nd(x, x_shape, nd=3):
# Convert a and b into 3 dimensional tensors.
a = flatten_to_nd(inputs[0], a_shape, 3)
b = flatten_to_nd(inputs[1], b_shape, 3)
- # Transpose matrix dimensions of b.
- b = _op.transpose(b, [0, 2, 1])
# Perform a batch matmul.
- output = _op.nn.batch_matmul(a, b)
+ output = _op.nn.batch_matmul(a, b, transpose_b=False)
Review comment:
Thanks! @ymwangg
Just a little concern about changing the default behavior of framework frontend, since currently the default topi schedule support for NN format is not as strong as the original NT one.
This may cause confusions to those who have used onnx frontend before or who is using onnx frontend now.
To give an example, I've added an extra config to TensorFlow frontend which uses the NT format by default but provides an option to use the normal format. I think that would be better before we have prepared a strong enough topi.
cc @comaniac
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jcf94 commented on a change in pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
Posted by GitBox <gi...@apache.org>.
jcf94 commented on a change in pull request #8607:
URL: https://github.com/apache/tvm/pull/8607#discussion_r680612911
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -770,10 +770,8 @@ def flatten_to_nd(x, x_shape, nd=3):
# Convert a and b into 3 dimensional tensors.
a = flatten_to_nd(inputs[0], a_shape, 3)
b = flatten_to_nd(inputs[1], b_shape, 3)
- # Transpose matrix dimensions of b.
- b = _op.transpose(b, [0, 2, 1])
# Perform a batch matmul.
- output = _op.nn.batch_matmul(a, b)
+ output = _op.nn.batch_matmul(a, b, transpose_b=False)
Review comment:
Thanks! @ymwangg
Just a little concern about changing the default behavior of framework frontend, since currently the default topi schedule support for NN format is not as strong as the original NT one.
This may cause confusions to those who have used onnx frontend before or who is using onnx frontend now.
To give an example, I've added an extra config to TensorFlow frontend which uses the NT format by default but provides an option to use the normal format. I think that would be better before we have prepared a strong enough topi.
p.s.: You see, I've also kept the default layout for `nn.batch_matmul` to be the original NT.
https://github.com/apache/tvm/blob/765397295410c7ec15a5df8be0df05c2e18d5ea0/python/tvm/relay/frontend/tensorflow_ops.py#L1191-L1199
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ymwangg commented on pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
Posted by GitBox <gi...@apache.org>.
ymwangg commented on pull request #8607:
URL: https://github.com/apache/tvm/pull/8607#issuecomment-893063069
@trevor-m @jcf94 Please review again when you get a chance.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] trevor-m merged pull request #8607: [TensorRT] Add transpose_a/b for TensorRT batch_matmul
Posted by GitBox <gi...@apache.org>.
trevor-m merged pull request #8607:
URL: https://github.com/apache/tvm/pull/8607
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org