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 2022/03/02 16:46:02 UTC
[GitHub] [tvm] anwang2009 opened a new pull request #10450: [ONNX] Add MatMulInteger importer
anwang2009 opened a new pull request #10450:
URL: https://github.com/apache/tvm/pull/10450
As title. Re-uses `QLinearMatMul`'s implementation. The only difference is that MatMulInteger accumulates into int32, not int8 or uint8.
cc @cconvey @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] masahi commented on a change in pull request #10450: [ONNX] Add MatMulInteger importer
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #10450:
URL: https://github.com/apache/tvm/pull/10450#discussion_r818009862
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -3935,6 +3935,58 @@ def try_resolve_to_const(x, dtype_override=None):
return y
+class MatMulInteger(OnnxOpConverter):
+ """Operator converter for MatMulInteger."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ a = inputs[0]
Review comment:
My impression was that `MatMulInteger` exactly corresponds to our `qnn.dense`. We can use `QLinearMatMul` converter but we want to skip requantize.
--
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] anwang2009 commented on pull request #10450: [ONNX] Add MatMulInteger importer
Posted by GitBox <gi...@apache.org>.
anwang2009 commented on pull request #10450:
URL: https://github.com/apache/tvm/pull/10450#issuecomment-1058580614
It looks like the onnx importer test is failing on `test_matmulinteger` for cuda, since the cuda schedule generation for `dense_int8` relies on the inner dimension of the multiply being a multiple of 4. The default onnx test's inner dimension is 3, and there is no generic schedule for non-4-multiples to fall back to. Should I close this PR until this issue can be addressed?
--
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 #10450: [ONNX] Add MatMulInteger importer
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on a change in pull request #10450:
URL: https://github.com/apache/tvm/pull/10450#discussion_r818281903
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -3935,6 +3935,58 @@ def try_resolve_to_const(x, dtype_override=None):
return y
+class MatMulInteger(OnnxOpConverter):
+ """Operator converter for MatMulInteger."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ a = inputs[0]
+ b = inputs[1]
+
+ a_dtype = infer_type(a).checked_type.dtype
+ b_dtype = infer_type(b).checked_type.dtype
+
+ assert a_dtype in ("int8", "uint8"), "MatMulInteger: invalid dtype for first input"
+ assert b_dtype in ("int8", "uint8"), "MatMulInteger: invalid dtype for second input"
+
+ assert a_dtype == b_dtype, "MatMulInteger: input dtypes must match"
+
+ a_scale = _op.const(1.0, dtype="float32")
+ b_scale = _op.const(1.0, dtype="float32")
+ out_scale = _op.const(1.0, dtype="float32")
+
+ a_zero_point = _op.const(0.0, dtype=a_dtype)
Review comment:
zero point does not seem to be fixed to 0? The scale points appear to be not present though which is weird. You might have to read the codebase and see what the intention really is since the documentation is confusing (and my guess has a mistake in it): https://github.com/onnx/onnx/blob/main/docs/Operators.md#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] AndrewZhaoLuo commented on a change in pull request #10450: [ONNX] Add MatMulInteger importer
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on a change in pull request #10450:
URL: https://github.com/apache/tvm/pull/10450#discussion_r817987993
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -3935,6 +3935,58 @@ def try_resolve_to_const(x, dtype_override=None):
return y
+class MatMulInteger(OnnxOpConverter):
+ """Operator converter for MatMulInteger."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ a = inputs[0]
Review comment:
Hmm so pretty sure you can just do `relay.nn.matmul(..., out_dtype='int32')`. You don't need all of this QLinearMatMul stuff to handle accumulation without overflow imo.
--
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 #10450: [ONNX] Add MatMulInteger importer
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on a change in pull request #10450:
URL: https://github.com/apache/tvm/pull/10450#discussion_r818281692
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -3935,6 +3935,58 @@ def try_resolve_to_const(x, dtype_override=None):
return y
+class MatMulInteger(OnnxOpConverter):
+ """Operator converter for MatMulInteger."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ a = inputs[0]
Review comment:
https://github.com/onnx/onnx/blob/main/docs/Operators.md#matmulinteger <-- I was fixated on the line where it said it was identical to numpy's. It appears the zero points are not fixed always so using QLinearMatMul is probably best choice.
--
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] anwang2009 commented on a change in pull request #10450: [ONNX] Add MatMulInteger importer
Posted by GitBox <gi...@apache.org>.
anwang2009 commented on a change in pull request #10450:
URL: https://github.com/apache/tvm/pull/10450#discussion_r818410945
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -3935,6 +3935,58 @@ def try_resolve_to_const(x, dtype_override=None):
return y
+class MatMulInteger(OnnxOpConverter):
+ """Operator converter for MatMulInteger."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ a = inputs[0]
+ b = inputs[1]
+
+ a_dtype = infer_type(a).checked_type.dtype
+ b_dtype = infer_type(b).checked_type.dtype
+
+ assert a_dtype in ("int8", "uint8"), "MatMulInteger: invalid dtype for first input"
+ assert b_dtype in ("int8", "uint8"), "MatMulInteger: invalid dtype for second input"
+
+ assert a_dtype == b_dtype, "MatMulInteger: input dtypes must match"
+
+ a_scale = _op.const(1.0, dtype="float32")
+ b_scale = _op.const(1.0, dtype="float32")
+ out_scale = _op.const(1.0, dtype="float32")
+
+ a_zero_point = _op.const(0.0, dtype=a_dtype)
Review comment:
yeah zp isn't necessarily fixed to 0, but the docs say 0 should be the default. I do this a few lines down:
```
if len(inputs) == 4:
a_zero_point = inputs[2]
b_zero_point = inputs[3]
```
From the example it looks like the scale factors are 1 by default.
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -3935,6 +3935,58 @@ def try_resolve_to_const(x, dtype_override=None):
return y
+class MatMulInteger(OnnxOpConverter):
+ """Operator converter for MatMulInteger."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ a = inputs[0]
Review comment:
modified `QLinearMatMul` converter to skip requantize in the MatMulInteger case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #10450: [ONNX] Add MatMulInteger importer
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #10450:
URL: https://github.com/apache/tvm/pull/10450#issuecomment-1058590459
I think there should be dense legalize or something that adds padding and slicing to handle such cases
--
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] mbrookhart commented on a change in pull request #10450: [ONNX] Add MatMulInteger importer
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #10450:
URL: https://github.com/apache/tvm/pull/10450#discussion_r817994425
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -3935,6 +3935,58 @@ def try_resolve_to_const(x, dtype_override=None):
return y
+class MatMulInteger(OnnxOpConverter):
+ """Operator converter for MatMulInteger."""
+
+ @classmethod
+ def _impl_v10(cls, inputs, attr, params):
+ a = inputs[0]
Review comment:
If scale is always 1 and zero point is always 0, I think that's true.
--
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