You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/01/02 23:04:19 UTC
[GitHub] [incubator-tvm] anijain2305 opened a new pull request #4611: {QNN]
Making scale/zero_points as expr instead of attrs.
anijain2305 opened a new pull request #4611: {QNN] Making scale/zero_points as expr instead of attrs.
URL: https://github.com/apache/incubator-tvm/pull/4611
Currently QNN dialect only deals with uniform quantization, which means each tensor has just one scale and zero point. Because of this restriction, QNN design had scale and zero points as op attributes. However, as we move towards channel quantization, scale will become a vector (and behave similar to something like bias for bias_add in terms of ops inputs).
Before moving to channel quantization, this PR makes the necessary changes to make the scale and zero points as input expr to operators (instead of making them op attrs). So, this PR does not bring any functional/performance change to QNN graphs. The new type checks still check that scale and zero points must be const scalar values. Once this PR is merged, and we start moving towards channel-wise quantization, we can start relaxing the checks and modifying the lowering.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] anijain2305 edited a comment on issue #4611: {QNN]
Making scale/zero_points as expr instead of attrs.
Posted by GitBox <gi...@apache.org>.
anijain2305 edited a comment on issue #4611: {QNN] Making scale/zero_points as expr instead of attrs.
URL: https://github.com/apache/incubator-tvm/pull/4611#issuecomment-570398052
Please review @zhiics @FrozenGene @jackwish
@u99127 You might be using QNN already. It might affect you if you are reading QNN graphs directly. Please review!
@vinx13 FixedPointMultiply is shared between QNN and Automatic Quantization. However, I deliberately kept FixedPointMultiply unchanged, so this PR does not affect automatic quantization. Please review!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] jackwish commented on a change in pull request
#4611: {QNN] Making scale/zero_points as expr instead of attrs.
Posted by GitBox <gi...@apache.org>.
jackwish commented on a change in pull request #4611: {QNN] Making scale/zero_points as expr instead of attrs.
URL: https://github.com/apache/incubator-tvm/pull/4611#discussion_r362707019
##########
File path: src/relay/qnn/op/concatenate.cc
##########
@@ -34,19 +34,48 @@ namespace tvm {
namespace relay {
namespace qnn {
-TVM_REGISTER_NODE_TYPE(QnnConcatenateAttrs);
-
-Expr MakeQnnConcatenate(Expr data, Array<tvm::Expr> input_scales,
- Array<tvm::Expr> input_zero_points, double output_scale,
- int32_t output_zero_point, int axis) {
- auto attrs = make_object<QnnConcatenateAttrs>();
- attrs->input_scales = std::move(input_scales);
- attrs->input_zero_points = std::move(input_zero_points);
- attrs->output_scale = output_scale;
- attrs->output_zero_point = output_zero_point;
+bool QnnConcatenateRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ CHECK_EQ(types.size(), 6);
+
+ // Check the scale and zero point types
+ const auto* input_scales_tuple = types[1].as<TupleTypeNode>();
+ if (input_scales_tuple == nullptr) {
+ throw relay::Error(
+ RELAY_ERROR("qnn concatenate requires a tuple of scales as the second argument, found "
+ << PrettyPrint(types[1])));
+ }
+ for (auto input_scale : input_scales_tuple->fields) {
Review comment:
could be `auto&`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] jackwish commented on a change in pull request
#4611: {QNN] Making scale/zero_points as expr instead of attrs.
Posted by GitBox <gi...@apache.org>.
jackwish commented on a change in pull request #4611: {QNN] Making scale/zero_points as expr instead of attrs.
URL: https://github.com/apache/incubator-tvm/pull/4611#discussion_r362704400
##########
File path: python/tvm/relay/qnn/op/legalizations.py
##########
@@ -18,9 +18,24 @@
"""Backend QNN related feature registration"""
from __future__ import absolute_import
+import numpy as np
import tvm
from tvm import relay
from .. import op as reg
+from ... import expr as _expr
+
+def get_scalar_from_constant(expr):
Review comment:
duplicated with the one in `frontend/common.py`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] jackwish commented on a change in pull request
#4611: {QNN] Making scale/zero_points as expr instead of attrs.
Posted by GitBox <gi...@apache.org>.
jackwish commented on a change in pull request #4611: {QNN] Making scale/zero_points as expr instead of attrs.
URL: https://github.com/apache/incubator-tvm/pull/4611#discussion_r362704054
##########
File path: python/tvm/relay/frontend/tflite.py
##########
@@ -224,9 +226,21 @@ def get_tensor_type_str(self, tensor_type):
raise NotImplementedError("Tensor type {} is currently not supported"
.format(str(tensor_type)))
+ def is_scalar(self, expr):
Review comment:
is anyone using 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] jackwish commented on a change in pull request
#4611: {QNN] Making scale/zero_points as expr instead of attrs.
Posted by GitBox <gi...@apache.org>.
jackwish commented on a change in pull request #4611: {QNN] Making scale/zero_points as expr instead of attrs.
URL: https://github.com/apache/incubator-tvm/pull/4611#discussion_r362707098
##########
File path: src/relay/qnn/op/concatenate.cc
##########
@@ -34,19 +34,48 @@ namespace tvm {
namespace relay {
namespace qnn {
-TVM_REGISTER_NODE_TYPE(QnnConcatenateAttrs);
-
-Expr MakeQnnConcatenate(Expr data, Array<tvm::Expr> input_scales,
- Array<tvm::Expr> input_zero_points, double output_scale,
- int32_t output_zero_point, int axis) {
- auto attrs = make_object<QnnConcatenateAttrs>();
- attrs->input_scales = std::move(input_scales);
- attrs->input_zero_points = std::move(input_zero_points);
- attrs->output_scale = output_scale;
- attrs->output_zero_point = output_zero_point;
+bool QnnConcatenateRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ CHECK_EQ(types.size(), 6);
+
+ // Check the scale and zero point types
+ const auto* input_scales_tuple = types[1].as<TupleTypeNode>();
+ if (input_scales_tuple == nullptr) {
+ throw relay::Error(
+ RELAY_ERROR("qnn concatenate requires a tuple of scales as the second argument, found "
+ << PrettyPrint(types[1])));
+ }
+ for (auto input_scale : input_scales_tuple->fields) {
+ CHECK(IsScalarType(input_scale, DataType::Float(32))); // input_scales[idx]
+ }
+
+ const auto* input_zero_points_tuple = types[2].as<TupleTypeNode>();
+ if (input_zero_points_tuple == nullptr) {
+ throw relay::Error(
+ RELAY_ERROR("qnn concatenate requires a tuple of zero_points as the third argument, found "
+ << PrettyPrint(types[2])));
+ }
+ for (auto input_zero_point : input_zero_points_tuple->fields) {
Review comment:
could be `auto&`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] vinx13 commented on issue #4611: {QNN] Making
scale/zero_points as expr instead of attrs.
Posted by GitBox <gi...@apache.org>.
vinx13 commented on issue #4611: {QNN] Making scale/zero_points as expr instead of attrs.
URL: https://github.com/apache/incubator-tvm/pull/4611#issuecomment-570574707
Thanks @anijain2305 @FrozenGene @Leo-arm @jackwish this is 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] Leo-arm commented on issue #4611: {QNN] Making
scale/zero_points as expr instead of attrs.
Posted by GitBox <gi...@apache.org>.
Leo-arm commented on issue #4611: {QNN] Making scale/zero_points as expr instead of attrs.
URL: https://github.com/apache/incubator-tvm/pull/4611#issuecomment-570538627
On behalf of @u99127, lgtm.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] FrozenGene commented on a change in pull request
#4611: {QNN] Making scale/zero_points as expr instead of attrs.
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on a change in pull request #4611: {QNN] Making scale/zero_points as expr instead of attrs.
URL: https://github.com/apache/incubator-tvm/pull/4611#discussion_r362713672
##########
File path: src/relay/qnn/op/concatenate.cc
##########
@@ -34,19 +34,48 @@ namespace tvm {
namespace relay {
namespace qnn {
-TVM_REGISTER_NODE_TYPE(QnnConcatenateAttrs);
-
-Expr MakeQnnConcatenate(Expr data, Array<tvm::Expr> input_scales,
- Array<tvm::Expr> input_zero_points, double output_scale,
- int32_t output_zero_point, int axis) {
- auto attrs = make_object<QnnConcatenateAttrs>();
- attrs->input_scales = std::move(input_scales);
- attrs->input_zero_points = std::move(input_zero_points);
- attrs->output_scale = output_scale;
- attrs->output_zero_point = output_zero_point;
+bool QnnConcatenateRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ CHECK_EQ(types.size(), 6);
+
+ // Check the scale and zero point types
+ const auto* input_scales_tuple = types[1].as<TupleTypeNode>();
+ if (input_scales_tuple == nullptr) {
+ throw relay::Error(
+ RELAY_ERROR("qnn concatenate requires a tuple of scales as the second argument, found "
+ << PrettyPrint(types[1])));
+ }
+ for (auto input_scale : input_scales_tuple->fields) {
+ CHECK(IsScalarType(input_scale, DataType::Float(32))); // input_scales[idx]
+ }
+
+ const auto* input_zero_points_tuple = types[2].as<TupleTypeNode>();
+ if (input_zero_points_tuple == nullptr) {
+ throw relay::Error(
+ RELAY_ERROR("qnn concatenate requires a tuple of zero_points as the third argument, found "
+ << PrettyPrint(types[2])));
+ }
+ for (auto input_zero_point : input_zero_points_tuple->fields) {
Review comment:
`const auto&` maybe better. We don't modify the value and just do check.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] anijain2305 edited a comment on issue #4611: {QNN]
Making scale/zero_points as expr instead of attrs.
Posted by GitBox <gi...@apache.org>.
anijain2305 edited a comment on issue #4611: {QNN] Making scale/zero_points as expr instead of attrs.
URL: https://github.com/apache/incubator-tvm/pull/4611#issuecomment-570398052
Please review @zhiics @FrozenGene @jackwish @shoubhik
@u99127 You might be using QNN already. It might affect you if you are reading QNN graphs directly. Please review!
@vinx13 FixedPointMultiply is shared between QNN and Automatic Quantization. However, I deliberately kept FixedPointMultiply unchanged, so this PR does not affect automatic quantization. Please review!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] anijain2305 commented on issue #4611: {QNN] Making
scale/zero_points as expr instead of attrs.
Posted by GitBox <gi...@apache.org>.
anijain2305 commented on issue #4611: {QNN] Making scale/zero_points as expr instead of attrs.
URL: https://github.com/apache/incubator-tvm/pull/4611#issuecomment-570398052
Please review @zhiics @FrozenGene @jackwish
@u99127 You might be using QNN already. It might affect you if you are reading QNN graphs directly. Please review!
@vinx13 FixedPointMultiply is shared between QNN and Automatic Quantization. However, I deliberately kept FixedPointMultiply unchanged, so it does not automatic quantization. Please review!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] vinx13 merged pull request #4611: {QNN] Making
scale/zero_points as expr instead of attrs.
Posted by GitBox <gi...@apache.org>.
vinx13 merged pull request #4611: {QNN] Making scale/zero_points as expr instead of attrs.
URL: https://github.com/apache/incubator-tvm/pull/4611
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services