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/11/22 12:02:07 UTC
[GitHub] [tvm] vvchernov opened a new pull request, #13469: WIP: [QNN] support zero points as variable scalar for QnnBatchMatMul op
vvchernov opened a new pull request, #13469:
URL: https://github.com/apache/tvm/pull/13469
There is fix of [the issue](https://github.com/apache/tvm/issues/13467)
cc @ibsidorenko @apeskov
--
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] vvchernov commented on pull request #13469: [QNN] support zero points as variable scalar for QnnBatchMatMul op
Posted by GitBox <gi...@apache.org>.
vvchernov commented on PR #13469:
URL: https://github.com/apache/tvm/pull/13469#issuecomment-1333286212
Hello @AndrewZhaoLuo! Thanks for help, the PR is ready for 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] ibsidorenko commented on a diff in pull request #13469: [QNN] support zero points as variable scalar for QnnBatchMatMul op
Posted by GitBox <gi...@apache.org>.
ibsidorenko commented on code in PR #13469:
URL: https://github.com/apache/tvm/pull/13469#discussion_r1029324848
##########
src/relay/qnn/op/batch_matmul.cc:
##########
@@ -96,20 +96,42 @@ Expr BatchMatmulFirstTerm(const Expr& quantized_x, const Expr& quantized_y,
}
Expr BatchMatmulSecondTerm(const Expr& x_quantized_data, const Expr& y_zero_point) {
- Array<Integer> axes = {2};
- return Multiply(y_zero_point, Sum(Cast(x_quantized_data, DataType::Int(32)), axes, true, false));
+ if (IsScalar(y_zero_point)) {
Review Comment:
```suggestion
ICHECK(IsScalar(y_zero_point)) << "Tensor ZP is not supported"
```
Do you think return empty Expr() is better choice? I am afraid it will be not clear the reason of fail when not scalar ZP will be provided.
--
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] vvchernov commented on a diff in pull request #13469: [QNN] support zero points as variable scalar for QnnBatchMatMul op
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #13469:
URL: https://github.com/apache/tvm/pull/13469#discussion_r1029731951
##########
src/relay/qnn/op/batch_matmul.cc:
##########
@@ -96,20 +96,42 @@ Expr BatchMatmulFirstTerm(const Expr& quantized_x, const Expr& quantized_y,
}
Expr BatchMatmulSecondTerm(const Expr& x_quantized_data, const Expr& y_zero_point) {
- Array<Integer> axes = {2};
- return Multiply(y_zero_point, Sum(Cast(x_quantized_data, DataType::Int(32)), axes, true, false));
+ if (IsScalar(y_zero_point)) {
Review Comment:
done
--
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] ibsidorenko commented on a diff in pull request #13469: [QNN] support zero points as variable scalar for QnnBatchMatMul op
Posted by GitBox <gi...@apache.org>.
ibsidorenko commented on code in PR #13469:
URL: https://github.com/apache/tvm/pull/13469#discussion_r1029324848
##########
src/relay/qnn/op/batch_matmul.cc:
##########
@@ -96,20 +96,42 @@ Expr BatchMatmulFirstTerm(const Expr& quantized_x, const Expr& quantized_y,
}
Expr BatchMatmulSecondTerm(const Expr& x_quantized_data, const Expr& y_zero_point) {
- Array<Integer> axes = {2};
- return Multiply(y_zero_point, Sum(Cast(x_quantized_data, DataType::Int(32)), axes, true, false));
+ if (IsScalar(y_zero_point)) {
Review Comment:
```suggestion
ICHECK(IsScalar(y_zero_point)) << "Tensor ZP is not supported";
```
Do you think return empty Expr() is better choice? I am afraid it will be not clear the reason of fail when not scalar ZP will be provided.
--
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] vvchernov commented on a diff in pull request #13469: [QNN] support zero points as variable scalar for QnnBatchMatMul op
Posted by GitBox <gi...@apache.org>.
vvchernov commented on code in PR #13469:
URL: https://github.com/apache/tvm/pull/13469#discussion_r1036051006
##########
src/relay/qnn/op/batch_matmul.cc:
##########
@@ -175,27 +200,32 @@ Expr QnnBatchMatmulCanonicalize(const Attrs& attrs, const Array<Expr>& new_args,
const auto* qnn_batch_matmul_attrs = attrs.as<BatchMatmulAttrs>();
- // Extract the integer zero points.
- auto y_zero_point_int = GetScalarFromConstant<int>(y_zero_point);
- auto x_zero_point_int = GetScalarFromConstant<int>(x_zero_point);
-
// Get all the terms as described in the comments.
auto term1 = BatchMatmulFirstTerm(quantized_x, quantized_y, qnn_batch_matmul_attrs);
auto term2 = BatchMatmulSecondTerm(quantized_x, y_zero_point);
auto term3 = BatchMatmulThirdTerm(quantized_y, x_zero_point, broadcast_dim_size);
- auto term4 = BatchMatmulFourthTerm(x_zero_point_int, y_zero_point_int, reduction_dim_size);
-
- // Combine those 4 terms depending on the zero points to get the best lowering.
- if (x_zero_point_int == 0 && y_zero_point_int == 0) {
- // term 2, 3 and 4 become zero.
- return term1;
- } else if (x_zero_point_int == 0 && y_zero_point_int != 0) {
- // term 3 and term 4 become zero.
- return Subtract(term1, term2);
- } else if (x_zero_point_int != 0 && y_zero_point_int == 0) {
- // term 2 and term 4 become zero.
- return Subtract(term1, term3);
+
+ // TODO(vvchernov): how do additional checs (equal) influence on performance?
+ if (IsConstScalar(x_zero_point) && IsConstScalar(y_zero_point)) {
+ // Extract the integer zero points.
+ auto y_zero_point_int = GetScalarFromConstant<int>(y_zero_point);
+ auto x_zero_point_int = GetScalarFromConstant<int>(x_zero_point);
+ auto term4 = BatchMatmulFourthTerm(x_zero_point_int, y_zero_point_int, reduction_dim_size);
+ // Combine those 4 terms depending on the zero points to get the best lowering.
+ if (x_zero_point_int == 0 && y_zero_point_int != 0) {
Review Comment:
Thanks for your remark! It was fixed
--
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 diff in pull request #13469: [QNN] support zero points as variable scalar for QnnBatchMatMul op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo commented on code in PR #13469:
URL: https://github.com/apache/tvm/pull/13469#discussion_r1035126503
##########
src/relay/qnn/op/batch_matmul.cc:
##########
@@ -175,27 +200,32 @@ Expr QnnBatchMatmulCanonicalize(const Attrs& attrs, const Array<Expr>& new_args,
const auto* qnn_batch_matmul_attrs = attrs.as<BatchMatmulAttrs>();
- // Extract the integer zero points.
- auto y_zero_point_int = GetScalarFromConstant<int>(y_zero_point);
- auto x_zero_point_int = GetScalarFromConstant<int>(x_zero_point);
-
// Get all the terms as described in the comments.
auto term1 = BatchMatmulFirstTerm(quantized_x, quantized_y, qnn_batch_matmul_attrs);
auto term2 = BatchMatmulSecondTerm(quantized_x, y_zero_point);
auto term3 = BatchMatmulThirdTerm(quantized_y, x_zero_point, broadcast_dim_size);
- auto term4 = BatchMatmulFourthTerm(x_zero_point_int, y_zero_point_int, reduction_dim_size);
-
- // Combine those 4 terms depending on the zero points to get the best lowering.
- if (x_zero_point_int == 0 && y_zero_point_int == 0) {
- // term 2, 3 and 4 become zero.
- return term1;
- } else if (x_zero_point_int == 0 && y_zero_point_int != 0) {
- // term 3 and term 4 become zero.
- return Subtract(term1, term2);
- } else if (x_zero_point_int != 0 && y_zero_point_int == 0) {
- // term 2 and term 4 become zero.
- return Subtract(term1, term3);
+
+ // TODO(vvchernov): how do additional checs (equal) influence on performance?
+ if (IsConstScalar(x_zero_point) && IsConstScalar(y_zero_point)) {
+ // Extract the integer zero points.
+ auto y_zero_point_int = GetScalarFromConstant<int>(y_zero_point);
+ auto x_zero_point_int = GetScalarFromConstant<int>(x_zero_point);
+ auto term4 = BatchMatmulFourthTerm(x_zero_point_int, y_zero_point_int, reduction_dim_size);
+ // Combine those 4 terms depending on the zero points to get the best lowering.
+ if (x_zero_point_int == 0 && y_zero_point_int != 0) {
Review Comment:
I believe some copy pasting was wrong here.
We want
`if (x_zero_point_int == 0 && y_zero_point_int == 0) {`
--
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] tvm-bot commented on pull request #13469: WIP: [QNN] support zero points as variable scalar for QnnBatchMatMul op
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13469:
URL: https://github.com/apache/tvm/pull/13469#issuecomment-1323558969
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<!--bot-comment-ccs-start-->
* No users to tag found in teams: `qnn` <sub>See [#10317](https://github.com/apache/tvm/issues/10317) for details</sub><!--bot-comment-ccs-end-->
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
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 merged pull request #13469: [QNN] support zero points as variable scalar for QnnBatchMatMul op
Posted by GitBox <gi...@apache.org>.
AndrewZhaoLuo merged PR #13469:
URL: https://github.com/apache/tvm/pull/13469
--
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