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/10/04 10:02:40 UTC
[GitHub] [tvm] abhikran-quic opened a new pull request #9186: [ONNX] Add MatMulInteger16 contrib op
abhikran-quic opened a new pull request #9186:
URL: https://github.com/apache/tvm/pull/9186
This PR adds support for contrib op: [com.microsoft.MatMulInteger16](https://github.com/microsoft/onnxruntime/blob/master/docs/ContribOperators.md#com.microsoft.MatMulInteger16).
Even though op definition tells that the supported input data types should be int16/uint16, the onnxruntime implements the op for signed integers only.
--
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] abhikran-quic commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r721402272
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +873,89 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ a_dtype = infer_type(inputs[0]).checked_type.dtype
+ b_dtype = infer_type(inputs[1]).checked_type.dtype
+ # Check input data types
+ assert a_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for first input"
+ assert b_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for second input"
+ out_dtype = "int32"
+ # Set output data type as uint32 when both inputs are uint16
+ if a_dtype == "uint16" and b_dtype == "uint16":
+ out_dtype = "uint32"
+ if a_rank > 2 or b_rank > 2:
+
+ def flatten_to_nd(x, x_shape, nd=3):
Review comment:
Yes. Agree with you. I will fix it in the next commit.
--
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] AndrewZhaoLuo commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r724389698
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +874,73 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ a_dtype = infer_type(inputs[0]).checked_type.dtype
Review comment:
To grab the rank and dtype you only need one pass of infer_type. infer_shape calls infer_type internally so you can get rid of a call by doing something like
a_relay_type = infer_type(a).checked_type
a_dtype = a_relay_type.dtype
a_rank = len(a_relay-type.shape)
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +874,73 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ a_dtype = infer_type(inputs[0]).checked_type.dtype
+ b_dtype = infer_type(inputs[1]).checked_type.dtype
+ # Check input data types
+ assert a_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for first input"
+ assert b_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for second input"
+ out_dtype = "int32"
+ if a_dtype == "uint16" and b_dtype == "uint16":
+ out_dtype = "uint32"
+ if a_rank > 2 or b_rank > 2:
+ b_type = infer_type(inputs[1])
Review comment:
If you do the above, this will no longer be needed to since you'll already have a variable in scope
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +874,73 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
Review comment:
A lot of this is just identical to MatMul except for the logic on types. Can you refactor Matmul to make it extensible and use it in this situation instead of copying most of the code?
--
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] abhikran-quic commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-974769683
> @abhikran-quic https://github.com/apache/tvm/pull/9540/files should fix this. You can take these changes in this PR and I can close mine.
Thank you @AndrewZhaoLuo for your help on this. I've incorporated the changes mentioned by you in this patch.
--
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] AndrewZhaoLuo commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-974512711
The error has to do with the return type. It expects an int16 return type, not int32.
It looks like there might be a problem with mixed precision support for batch_matmul on cuda. Looking into it.
--
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] AndrewZhaoLuo commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-948056581
@abhikran-quic do you still plan on working on this?
--
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] abhikran-quic commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r722466407
##########
File path: tests/python/frontend/onnx/test_forward.py
##########
@@ -1282,6 +1282,47 @@ def verify_batch_matmul(a_shape, b_shape, out_shape, convert_config=None):
)
+@tvm.testing.parametrize_targets
+def test_matmulinteger16(target, dev):
+ def verify_matmulinteger16(a_shape, b_shape, out_shape):
+ a_dtype = "int16"
+ b_dtype = "int16"
+ low = -10
+ high = 10
Review comment:
Since the op can be used for signed/unsigned integers, the range is kept as [-10, 10] to cover common inputs.
--
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] cconvey commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
cconvey commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r721398872
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +873,89 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ a_dtype = infer_type(inputs[0]).checked_type.dtype
+ b_dtype = infer_type(inputs[1]).checked_type.dtype
+ # Check input data types
+ assert a_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for first input"
+ assert b_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for second input"
+ out_dtype = "int32"
+ # Set output data type as uint32 when both inputs are uint16
+ if a_dtype == "uint16" and b_dtype == "uint16":
+ out_dtype = "uint32"
+ if a_rank > 2 or b_rank > 2:
+
+ def flatten_to_nd(x, x_shape, nd=3):
Review comment:
Would it possibly make sense to move that function outside of `batch_matmul`, and then you could just call that existing implementation? It would be nice to avoid duplicating that much code.
--
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] cconvey commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
cconvey commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r722481055
##########
File path: tests/python/frontend/onnx/test_forward.py
##########
@@ -1282,6 +1282,47 @@ def verify_batch_matmul(a_shape, b_shape, out_shape, convert_config=None):
)
+@tvm.testing.parametrize_targets
+def test_matmulinteger16(target, dev):
+ def verify_matmulinteger16(a_shape, b_shape, out_shape):
+ a_dtype = "int16"
+ b_dtype = "int16"
+ low = -10
+ high = 10
Review comment:
Or maybe use `np.iinfo(np.int16).min` and `...max` as the bounds?
If the goal is to increase the chances of randomly discovering latent bugs, and we don't know the _specific_ values at which corner-cases happen, perhaps it makes sense to at least range over all possible values.
I'm not sure what the best strategy is here, but I'm wondering if this is a step in the right direction.
--
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] AndrewZhaoLuo commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-975882822
Ah yes, MatMulInteger is an onnx op but it is seperate from MatMulInteger16
--
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] Icemist commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
Icemist commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-1008482107
@abhikran-quic
Could I please ask why did you delete the code branch under the ONNX_DEFAULT_CONFIGS["use_nt_batch_matmul"] parameter?
I found that after this change, some of the code samples stopped working with an error like _“Check failed: (reporter->AssertEQ(xk, yk)) is false: BatchDot: shapes of x and y is inconsistent, x shape=[16, 384, 384], y shape=[16, 384, 64]“_
So now I see ONNX_DEFAULT_CONFIGS is not used by anyone. But The same logic with use_nt_batch_matmul is there for tensorflow.
Can we restore this logic? Will it affect the MatMulInteger16 operation you added?
--
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] cconvey commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
cconvey commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r721337230
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +873,89 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ a_dtype = infer_type(inputs[0]).checked_type.dtype
+ b_dtype = infer_type(inputs[1]).checked_type.dtype
+ # Check input data types
+ assert a_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for first input"
+ assert b_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for second input"
+ out_dtype = "int32"
+ # Set output data type as uint32 when both inputs are uint16
Review comment:
I'm not sure this particular comment is necessary, because the code it describes is already very readable.
--
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] abhikran-quic commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r721392405
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +873,89 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ a_dtype = infer_type(inputs[0]).checked_type.dtype
+ b_dtype = infer_type(inputs[1]).checked_type.dtype
+ # Check input data types
+ assert a_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for first input"
+ assert b_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for second input"
+ out_dtype = "int32"
+ # Set output data type as uint32 when both inputs are uint16
+ if a_dtype == "uint16" and b_dtype == "uint16":
+ out_dtype = "uint32"
+ if a_rank > 2 or b_rank > 2:
+
+ def flatten_to_nd(x, x_shape, nd=3):
Review comment:
I reused this function from `batch_matmul`.
An alternative to this could be to not set `nd `value while calling `flatten_to_nd`
--
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] AndrewZhaoLuo commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-949299164
No problem, have a good vacation!
--
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] abhikran-quic edited a comment on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic edited a comment on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-974769683
> @abhikran-quic https://github.com/apache/tvm/pull/9540/files should fix this. You can take these changes in this PR and I can close mine.
Thank you @AndrewZhaoLuo for your help on this. I've incorporated the changes mentioned by you in the latest patch.
--
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] tmoreau89 commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-934585560
Thanks for helping with the review @cconvey ! CC-ing @mbrookhart @anwang2009 @AndrewZhaoLuo
--
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] abhikran-quic commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-971543884
> @abhikran-quic friendly ping to see if updating this PR is currently blocked, thanks!
Hi @tmoreau89 , Thanks! I have a working version ready for the op. I should have it ready for review in couple of days.
--
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] abhikran-quic commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r733260630
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +874,73 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
Review comment:
Sure. I'll rework on this as you've requested.
--
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] tmoreau89 merged pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
tmoreau89 merged pull request #9186:
URL: https://github.com/apache/tvm/pull/9186
--
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] cconvey commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
cconvey commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r722475944
##########
File path: tests/python/frontend/onnx/test_forward.py
##########
@@ -1282,6 +1282,47 @@ def verify_batch_matmul(a_shape, b_shape, out_shape, convert_config=None):
)
+@tvm.testing.parametrize_targets
+def test_matmulinteger16(target, dev):
+ def verify_matmulinteger16(a_shape, b_shape, out_shape):
+ a_dtype = "int16"
+ b_dtype = "int16"
+ low = -10
+ high = 10
Review comment:
Would it makes sense to add a comment saying that these are semi-arbitrary values? It might help readers avoid looking for a deeper meaning for why these particular numbers were chosen.
--
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] cconvey commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
cconvey commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r721339933
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +873,89 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ a_dtype = infer_type(inputs[0]).checked_type.dtype
+ b_dtype = infer_type(inputs[1]).checked_type.dtype
+ # Check input data types
+ assert a_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for first input"
+ assert b_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for second input"
+ out_dtype = "int32"
+ # Set output data type as uint32 when both inputs are uint16
+ if a_dtype == "uint16" and b_dtype == "uint16":
+ out_dtype = "uint32"
+ if a_rank > 2 or b_rank > 2:
+
+ def flatten_to_nd(x, x_shape, nd=3):
Review comment:
nitpick: All 3 calls to this function explicitly specify the `nd` value. Does it still make sense to provide a default value here?
--
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] cconvey commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
cconvey commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r722481055
##########
File path: tests/python/frontend/onnx/test_forward.py
##########
@@ -1282,6 +1282,47 @@ def verify_batch_matmul(a_shape, b_shape, out_shape, convert_config=None):
)
+@tvm.testing.parametrize_targets
+def test_matmulinteger16(target, dev):
+ def verify_matmulinteger16(a_shape, b_shape, out_shape):
+ a_dtype = "int16"
+ b_dtype = "int16"
+ low = -10
+ high = 10
Review comment:
Or maybe use `np.iinfo(np.int16).min` and `...max` as the bounds?
If the goal is to increase the random discovery of latent bugs, and we don't know the _specific_ values at which corner-cases happen, perhaps it makes sense to at least range over all possible values.
I'm not sure what the best strategy is here, but I'm wondering if this is a step in the right direction.
--
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] cconvey commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
cconvey commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r722445625
##########
File path: tests/python/frontend/onnx/test_forward.py
##########
@@ -1282,6 +1282,47 @@ def verify_batch_matmul(a_shape, b_shape, out_shape, convert_config=None):
)
+@tvm.testing.parametrize_targets
+def test_matmulinteger16(target, dev):
+ def verify_matmulinteger16(a_shape, b_shape, out_shape):
+ a_dtype = "int16"
+ b_dtype = "int16"
+ low = -10
+ high = 10
Review comment:
Is there a particular reason for making the bounds -10 ... 10?
--
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] tmoreau89 commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-969396752
@abhikran-quic friendly ping to see if updating this PR is currently blocked, thanks!
--
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] AndrewZhaoLuo commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-974513979
--
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] abhikran-quic commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-976468201
> LGTM
Thank you @AndrewZhaoLuo for your review.
I would request reviewers to please review 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] AndrewZhaoLuo commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-974524662
I suggest a workaround by casting the inputs to int32 and then running things. This isn't a true representation of mixed precision but oh well, this issue is a bit trickier and kind of perplexing. In the mean time I will create an issue and you can reference it.
--
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] AndrewZhaoLuo commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-974528214
Actually it seems like this is quite simple, out_dtype is not getting passed in
--
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] abhikran-quic commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r721390543
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +873,89 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ a_dtype = infer_type(inputs[0]).checked_type.dtype
+ b_dtype = infer_type(inputs[1]).checked_type.dtype
+ # Check input data types
+ assert a_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for first input"
+ assert b_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for second input"
+ out_dtype = "int32"
+ # Set output data type as uint32 when both inputs are uint16
Review comment:
Sure. I will remove it in the next commit.
--
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] abhikran-quic commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r721531957
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +873,89 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ a_dtype = infer_type(inputs[0]).checked_type.dtype
+ b_dtype = infer_type(inputs[1]).checked_type.dtype
+ # Check input data types
+ assert a_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for first input"
+ assert b_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for second input"
+ out_dtype = "int32"
+ # Set output data type as uint32 when both inputs are uint16
+ if a_dtype == "uint16" and b_dtype == "uint16":
+ out_dtype = "uint32"
+ if a_rank > 2 or b_rank > 2:
+
+ def flatten_to_nd(x, x_shape, nd=3):
Review comment:
This is fixed in the following commit: `ccbe433937c0661414a5e62e1f6321058057e158`
--
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] abhikran-quic commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r721892645
##########
File path: tests/python/frontend/onnx/test_forward.py
##########
@@ -1282,6 +1282,45 @@ def verify_batch_matmul(a_shape, b_shape, out_shape, convert_config=None):
)
+@tvm.testing.parametrize_targets
+def test_matmulinteger16(target, dev):
+ def verify_matmulinteger16(a_shape, b_shape, out_shape):
+ a_dtype = "int16"
+ b_dtype = "int16"
+ low = -10
+ high = 10
+
+ a_proto = TensorProto.INT16
+ b_proto = TensorProto.INT16
+ out_proto = TensorProto.INT32
+ a_array = np.random.randint(low, high, size=a_shape).astype(a_dtype)
+ b_array = np.random.randint(low, high, size=b_shape).astype(b_dtype)
+
+ mul_node = helper.make_node("MatMulInteger16", ["a", "b"], ["out"], domain="com.microsoft")
+
+ graph = helper.make_graph(
+ [mul_node],
+ "matmuli16_test",
+ inputs=[
+ helper.make_tensor_value_info("a", a_proto, list(a_shape)),
+ helper.make_tensor_value_info("b", b_proto, list(b_shape)),
+ ],
+ outputs=[helper.make_tensor_value_info("out", out_proto, list(out_shape))],
+ )
+
+ model = helper.make_model(graph, producer_name="matmuli16_test")
+ verify_with_ort_with_inputs(model, [a_array, b_array], target=target, dev=dev)
+
+ # Working tests
Review comment:
Sure. I've added comments in the latest commit.
--
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] abhikran-quic commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r733260630
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +874,73 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
Review comment:
Sure. I'll rework on this as you've suggested.
--
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] abhikran-quic commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-977883549
Hello Reviewers,
Could you please review this PR for any more changes needed ?
--
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] abhikran-quic edited a comment on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic edited a comment on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-974769683
> @abhikran-quic https://github.com/apache/tvm/pull/9540/files should fix this. You can take these changes in this PR and I can close mine.
Thank you @AndrewZhaoLuo for your help on this. I've incorporated the changes mentioned by you in the latest patch.
However, there's one more pending issue. After removing test_matmulinteger from unsupported_onnx_tests, I saw an error from [onnx.py](https://github.com/apache/tvm/blob/main/python/tvm/relay/frontend/onnx.py#L4557) . Here is the link to [CI](https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-9186/10/pipeline/) where failure is seen.
IMHO, when we remove test_matmulinteger the error is expected because there is no op named matmulinteger in onnx.py. Onnxruntime supports [MatMulInteger16 ](https://github.com/microsoft/onnxruntime/blob/master/docs/ContribOperators.md#com.microsoft.MatMulInteger16) and [MatMulIntegerToFloat](https://github.com/microsoft/onnxruntime/blob/master/docs/ContribOperators.md#com.microsoft.MatMulIntegerToFloat) and hence we can add ops with the names mentioned in Onnx runtime documentation. Hence, removing test_matmulinteger is leading to the error.
In my latest patch, I've retained test_matmulinteger in unsupported_onnx_tests. Please share your thoughts on this.
--
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] abhikran-quic commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-973765700
> > I'll try to take a look at this error tomorrow abhikran
>
> Thank you @AndrewZhaoLuo for offering to help! I've uploaded new changes today and we will have to monitor if the same error is still observed. I will let you know in case it needs inputs from you. Also, could you please review the new changes that I've added today ?
Hi @AndrewZhaoLuo , The same error shows up again: https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-9186/8/pipeline
Could you please help in sharing insights on what could be the problem here ?
--
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] abhikran-quic edited a comment on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic edited a comment on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-948182531
> @abhikran-quic do you still plan on working on this?
Hi @AndrewZhaoLuo : I've been out of office since last two weeks and hence I've not been able to reply here. Sorry about this!
I will fix the changes that you've suggested once I'm back.
--
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] abhikran-quic commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-948182531
> @abhikran-quic do you still plan on working on this?
Hi @AndrewZhaoLuo : I've been out of office since last two weeks and hence I've not been able to reply here. Sorry about this!
I will fix the changes that you've requested once I'm back.
--
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] abhikran-quic commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-1009676481
Hi @Icemist , This change was done to optimize the code path and since all the tests in automation passed, we went ahead and committed the change.
If this is affecting the logic you've mentioned above, we can restore it definitely.
--
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] abhikran-quic commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r723118478
##########
File path: tests/python/frontend/onnx/test_forward.py
##########
@@ -1282,6 +1282,47 @@ def verify_batch_matmul(a_shape, b_shape, out_shape, convert_config=None):
)
+@tvm.testing.parametrize_targets
+def test_matmulinteger16(target, dev):
+ def verify_matmulinteger16(a_shape, b_shape, out_shape):
+ a_dtype = "int16"
+ b_dtype = "int16"
+ low = -10
+ high = 10
Review comment:
Yes. It would be a good idea to use` np.iinfo(np.int16).min` and `np.iinfo(np.int16).max` in the tests.
I tried it in my op and the tests are passing. I will fix this in an upcoming patch.
--
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] AndrewZhaoLuo commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r724389698
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +874,73 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ a_dtype = infer_type(inputs[0]).checked_type.dtype
Review comment:
To grab the rank and dtype you only need one pass of infer_type. infer_shape calls infer_type internally so you can get rid of a call by doing something like
a_relay_type = infer_type(a).checked_type
a_dtype = a_relay_type.dtype
a_rank = len(a_relay-type.shape)
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +874,73 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ a_dtype = infer_type(inputs[0]).checked_type.dtype
+ b_dtype = infer_type(inputs[1]).checked_type.dtype
+ # Check input data types
+ assert a_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for first input"
+ assert b_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for second input"
+ out_dtype = "int32"
+ if a_dtype == "uint16" and b_dtype == "uint16":
+ out_dtype = "uint32"
+ if a_rank > 2 or b_rank > 2:
+ b_type = infer_type(inputs[1])
Review comment:
If you do the above, this will no longer be needed to since you'll already have a variable in scope
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +874,73 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
Review comment:
A lot of this is just identical to MatMul except for the logic on types. Can you refactor Matmul to make it extensible and use it in this situation instead of copying most of the code?
--
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] abhikran-quic edited a comment on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic edited a comment on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-971543884
> @abhikran-quic friendly ping to see if updating this PR is currently blocked, thanks!
Hi @tmoreau89 , Thanks! I have a working version ready for the op as suggested by @AndrewZhaoLuo . I should have it ready for review in couple of days.
Though, I'd still need some help in figuring out the error seen on GPU tests for the op.
I've verified the tests on for X86 and ARM architectures but since I don't have a GPU, I am unable to run CUDA tests.
My observations regarding the error are mentioned below:
`Check failed: ret == 0 (-1 vs. 0) : Assert fail: (((tir.tvm_struct_get(arg2, 0, 5) == (uint8)0) && (tir.tvm_struct_get(arg2, 0, 6) == (uint8)16)) && (tir.tvm_struct_get(arg2, 0, 7) == (uint16)1)), arg2.dtype is expected to be int16
`
1. Here arg2 corresponds to output array. The output dtype is expected to be int16 whereas I'm explicitly specifying the output dtype as uint32 to batch_matmul: https://github.com/apache/tvm/pull/9186/files#diff-023ca730d77056c7a76a1c9b7f4f3f41ec985f8ec58cb346129925c7078dce7aR907
2. The relay strategy of batch_matmul op for X86 and CUDA are very different. Could this be a reason why output dtype is being altered by CUDA tests ?
3. Is there a way to run this test on x86 ?
--
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] AndrewZhaoLuo commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-974478344
Taking a look now
--
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] AndrewZhaoLuo commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r753567712
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -212,6 +212,77 @@ def get_scalar(x, params, dtype="float32"):
return _op.cast(x, dtype)
+def matmul_out_dtype(inputs, out_dtype):
+ """Common function to handle MatMul and MatMulInteger16"""
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ if a_rank > 2 or b_rank > 2:
+
+ def flatten_to_nd(x, x_shape, nd=3):
+ ndims = infer_shape(x_shape)[0]
+ if ndims == nd:
+ return x
+ newshape = _op.concatenate(
+ [
+ _expr.const([-1], dtype=infer_type(x_shape).checked_type.dtype),
+ _op.strided_slice(x_shape, [ndims - nd + 1], [ndims]),
+ ],
+ 0,
+ )
+ out = _op.reshape(x, fold_constant(newshape))
+ return out
+
+ b_type = infer_type(inputs[1])
+ # Convert to dense if the second matrix is 2d and non-dynamic
+ if b_rank == 2 and not _ty.is_dynamic(b_type.checked_type):
+ a = flatten_to_nd(inputs[0], a_shape, 2)
+ b = _op.transpose(inputs[1])
+ output = _op.nn.dense(a, b, out_dtype=out_dtype)
+ else:
+ # 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)
+ # Perform a NN batch matmul.
+ output = _op.nn.batch_matmul(a, b, out_dtype=out_dtype, transpose_b=False)
+ # Determine the output batch dimension.
+ if a_rank > b_rank:
+ out_batch = _op.strided_slice(a_shape, [0], [a_rank - 2])
+ elif a_rank < b_rank:
+ out_batch = _op.strided_slice(b_shape, [0], [b_rank - 2])
+ # If its unclear how broadcasting should be applied, the output
+ # shape is determined by choosing the maximum value from each input.
+ else:
+ out_batch = _op.concatenate(
+ [
+ _op.maximum(
+ _op.strided_slice(a_shape, [i], [i + 1]),
+ _op.strided_slice(b_shape, [i], [i + 1]),
+ )
+ for i in range(a_rank - 2)
+ ],
+ 0,
+ )
+ # Reshape output to original dimensions.
+ final_shape = _op.concatenate(
+ [
+ out_batch,
+ _op.strided_slice(
+ a_shape, [infer_shape(a_shape)[0] - 2], [infer_shape(a_shape)[0] - 1]
+ ),
+ _op.strided_slice(
+ b_shape, [infer_shape(b_shape)[0] - 1], [infer_shape(b_shape)[0]]
+ ),
Review comment:
in unsupported_onnx_tests can you remove test_matmulinteger
--
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] abhikran-quic commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r753760519
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -212,6 +212,77 @@ def get_scalar(x, params, dtype="float32"):
return _op.cast(x, dtype)
+def matmul_out_dtype(inputs, out_dtype):
+ """Common function to handle MatMul and MatMulInteger16"""
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ if a_rank > 2 or b_rank > 2:
+
+ def flatten_to_nd(x, x_shape, nd=3):
+ ndims = infer_shape(x_shape)[0]
+ if ndims == nd:
+ return x
+ newshape = _op.concatenate(
+ [
+ _expr.const([-1], dtype=infer_type(x_shape).checked_type.dtype),
+ _op.strided_slice(x_shape, [ndims - nd + 1], [ndims]),
+ ],
+ 0,
+ )
+ out = _op.reshape(x, fold_constant(newshape))
+ return out
+
+ b_type = infer_type(inputs[1])
+ # Convert to dense if the second matrix is 2d and non-dynamic
+ if b_rank == 2 and not _ty.is_dynamic(b_type.checked_type):
+ a = flatten_to_nd(inputs[0], a_shape, 2)
+ b = _op.transpose(inputs[1])
+ output = _op.nn.dense(a, b, out_dtype=out_dtype)
+ else:
+ # 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)
+ # Perform a NN batch matmul.
+ output = _op.nn.batch_matmul(a, b, out_dtype=out_dtype, transpose_b=False)
+ # Determine the output batch dimension.
+ if a_rank > b_rank:
+ out_batch = _op.strided_slice(a_shape, [0], [a_rank - 2])
+ elif a_rank < b_rank:
+ out_batch = _op.strided_slice(b_shape, [0], [b_rank - 2])
+ # If its unclear how broadcasting should be applied, the output
+ # shape is determined by choosing the maximum value from each input.
+ else:
+ out_batch = _op.concatenate(
+ [
+ _op.maximum(
+ _op.strided_slice(a_shape, [i], [i + 1]),
+ _op.strided_slice(b_shape, [i], [i + 1]),
+ )
+ for i in range(a_rank - 2)
+ ],
+ 0,
+ )
+ # Reshape output to original dimensions.
+ final_shape = _op.concatenate(
+ [
+ out_batch,
+ _op.strided_slice(
+ a_shape, [infer_shape(a_shape)[0] - 2], [infer_shape(a_shape)[0] - 1]
+ ),
+ _op.strided_slice(
+ b_shape, [infer_shape(b_shape)[0] - 1], [infer_shape(b_shape)[0]]
+ ),
Review comment:
Sure. This is resolved.
--
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] AndrewZhaoLuo commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-974535501
@abhikran-quic https://github.com/apache/tvm/pull/9540/files should fix this. You can take these changes in this PR and I can close mine.
--
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] abhikran-quic commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r753760519
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -212,6 +212,77 @@ def get_scalar(x, params, dtype="float32"):
return _op.cast(x, dtype)
+def matmul_out_dtype(inputs, out_dtype):
+ """Common function to handle MatMul and MatMulInteger16"""
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ if a_rank > 2 or b_rank > 2:
+
+ def flatten_to_nd(x, x_shape, nd=3):
+ ndims = infer_shape(x_shape)[0]
+ if ndims == nd:
+ return x
+ newshape = _op.concatenate(
+ [
+ _expr.const([-1], dtype=infer_type(x_shape).checked_type.dtype),
+ _op.strided_slice(x_shape, [ndims - nd + 1], [ndims]),
+ ],
+ 0,
+ )
+ out = _op.reshape(x, fold_constant(newshape))
+ return out
+
+ b_type = infer_type(inputs[1])
+ # Convert to dense if the second matrix is 2d and non-dynamic
+ if b_rank == 2 and not _ty.is_dynamic(b_type.checked_type):
+ a = flatten_to_nd(inputs[0], a_shape, 2)
+ b = _op.transpose(inputs[1])
+ output = _op.nn.dense(a, b, out_dtype=out_dtype)
+ else:
+ # 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)
+ # Perform a NN batch matmul.
+ output = _op.nn.batch_matmul(a, b, out_dtype=out_dtype, transpose_b=False)
+ # Determine the output batch dimension.
+ if a_rank > b_rank:
+ out_batch = _op.strided_slice(a_shape, [0], [a_rank - 2])
+ elif a_rank < b_rank:
+ out_batch = _op.strided_slice(b_shape, [0], [b_rank - 2])
+ # If its unclear how broadcasting should be applied, the output
+ # shape is determined by choosing the maximum value from each input.
+ else:
+ out_batch = _op.concatenate(
+ [
+ _op.maximum(
+ _op.strided_slice(a_shape, [i], [i + 1]),
+ _op.strided_slice(b_shape, [i], [i + 1]),
+ )
+ for i in range(a_rank - 2)
+ ],
+ 0,
+ )
+ # Reshape output to original dimensions.
+ final_shape = _op.concatenate(
+ [
+ out_batch,
+ _op.strided_slice(
+ a_shape, [infer_shape(a_shape)[0] - 2], [infer_shape(a_shape)[0] - 1]
+ ),
+ _op.strided_slice(
+ b_shape, [infer_shape(b_shape)[0] - 1], [infer_shape(b_shape)[0]]
+ ),
Review comment:
Sure. This is resolved.
--
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] tmoreau89 commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-978052829
Thank you @abhikran-quic, @AndrewZhaoLuo, @cconvey the PR has been merged
--
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] abhikran-quic commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-974769683
> @abhikran-quic https://github.com/apache/tvm/pull/9540/files should fix this. You can take these changes in this PR and I can close mine.
Thank you @AndrewZhaoLuo for your help on this. I've incorporated the changes mentioned by you in this patch.
--
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] AndrewZhaoLuo commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-972410653
I'll try to take a look at this error tomorrow abhikran
--
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] abhikran-quic commented on pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#issuecomment-973000513
> I'll try to take a look at this error tomorrow abhikran
Thank you @AndrewZhaoLuo for offering to help! I've uploaded new changes today and we will have to monitor if the same error is still observed. I will let you know in case it needs inputs from you.
Also, could you please review the new changes that I've added today ?
--
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] cconvey commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
cconvey commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r721539028
##########
File path: tests/python/frontend/onnx/test_forward.py
##########
@@ -1282,6 +1282,45 @@ def verify_batch_matmul(a_shape, b_shape, out_shape, convert_config=None):
)
+@tvm.testing.parametrize_targets
+def test_matmulinteger16(target, dev):
+ def verify_matmulinteger16(a_shape, b_shape, out_shape):
+ a_dtype = "int16"
+ b_dtype = "int16"
+ low = -10
+ high = 10
+
+ a_proto = TensorProto.INT16
+ b_proto = TensorProto.INT16
+ out_proto = TensorProto.INT32
+ a_array = np.random.randint(low, high, size=a_shape).astype(a_dtype)
+ b_array = np.random.randint(low, high, size=b_shape).astype(b_dtype)
+
+ mul_node = helper.make_node("MatMulInteger16", ["a", "b"], ["out"], domain="com.microsoft")
+
+ graph = helper.make_graph(
+ [mul_node],
+ "matmuli16_test",
+ inputs=[
+ helper.make_tensor_value_info("a", a_proto, list(a_shape)),
+ helper.make_tensor_value_info("b", b_proto, list(b_shape)),
+ ],
+ outputs=[helper.make_tensor_value_info("out", out_proto, list(out_shape))],
+ )
+
+ model = helper.make_model(graph, producer_name="matmuli16_test")
+ verify_with_ort_with_inputs(model, [a_array, b_array], target=target, dev=dev)
+
+ # Working tests
Review comment:
Not sure if I'm in the minority, but it's not obvious why the particular tests below were chosen. So it's hard to notice if there are any redundancies or gaps in the coverage they provide. Would it make sense to add some comments indicating the purpose some/all of them serve?
--
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] abhikran-quic commented on a change in pull request #9186: [ONNX] Add MatMulInteger16 contrib op
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on a change in pull request #9186:
URL: https://github.com/apache/tvm/pull/9186#discussion_r721392405
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -873,6 +873,89 @@ def flatten_to_nd(x, x_shape, nd=3):
return _op.nn.dense(inputs[0], input_1_t)
+class MatMulInteger16(OnnxOpConverter):
+ """Operator converter for MatMulInteger16 from Microsoft onnxruntime contrib opset."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ assert len(inputs) == 2, "MatMul op take 2 inputs, {} given".format(len(inputs))
+ a_shape = shape_of(inputs[0])
+ a_rank = infer_shape(a_shape)[0]
+ b_shape = shape_of(inputs[1])
+ b_rank = infer_shape(b_shape)[0]
+ a_dtype = infer_type(inputs[0]).checked_type.dtype
+ b_dtype = infer_type(inputs[1]).checked_type.dtype
+ # Check input data types
+ assert a_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for first input"
+ assert b_dtype in ("int16", "uint16"), "MatMulInteger16: invalid dtype for second input"
+ out_dtype = "int32"
+ # Set output data type as uint32 when both inputs are uint16
+ if a_dtype == "uint16" and b_dtype == "uint16":
+ out_dtype = "uint32"
+ if a_rank > 2 or b_rank > 2:
+
+ def flatten_to_nd(x, x_shape, nd=3):
Review comment:
I reused this function from `batch_matmul`.
An alternative to this could be to not set `nd `value(when it's supposed to be 3) while calling `flatten_to_nd`
--
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