Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finalize the semantics of storage_min, storage_max #1406

Closed
sdasgup3 opened this issue Apr 14, 2023 · 7 comments
Closed

Finalize the semantics of storage_min, storage_max #1406

sdasgup3 opened this issue Apr 14, 2023 · 7 comments
Assignees

Comments

@sdasgup3
Copy link
Member

sdasgup3 commented Apr 14, 2023

The goal of this ticket is to investigate whether or not the quantized type specification should limit the allowed values of storage_min and storage_max. Currently, the allowed values are limited to [min_value(storage_type, max_value(storage_type], but this limit may not be followed by some use cases.

Please refer to ref to follow the discussion.

update
The current spec expects default values of min_value(storage_type) and max_value(storage_type) for storage_min and storage_max resp., if they are not explicitly provided. Moreover, the specification documents the following general constraint on storage_min and storage_max

`min_value(storage_type) <= storage_min < storage_max <= max_value(storage_type)`.
@sdasgup3 sdasgup3 added the Spec label Apr 14, 2023
@sdasgup3 sdasgup3 self-assigned this Apr 14, 2023
@sdasgup3 sdasgup3 changed the title Figuring out the values for storage_min, storage_max Figuring out the limits for storage_min, storage_max Apr 14, 2023
@burmako burmako changed the title Figuring out the limits for storage_min, storage_max Finalize the semantics of storage_min, storage_max Apr 14, 2023
@burmako burmako moved this to Todo in Frontend contract Apr 23, 2023
@sdasgup3
Copy link
Member Author

sdasgup3 commented Apr 28, 2023

From the POV of the semantics of an op producing quantized results, the relevance storage_min/storage_max values are  to clamp the result(s) . The current spec expects default values of min_value(storage_type) and max_value(storage_type) for storage_min and storage_max resp., if they are not explicitly provided. Moreover, the specification documents the following general constraint on storage_min and storage_max

`min_value(storage_type) <= storage_min < storage_max <= max_value(storage_type)`.

My personal opinion: The above constraint seems sufficiently general to accommodate the existing implementations (e.g. TFLite) as well as allow  the adoption of new or experimental quantization schemes.  The op semantics should not add additional constraints unless there is a strong reason to do that.

Having said that, let me I am re-proposing a draft PR for the specification of quantized add. As some of you might remember that, at some point, we decided to drop the introduction of its specification mainly because we were unsure about the fate of #1406.

The proposal will be almost same as before except that it does not have any additional constraint imposed by the op's semantics on storage_min or storage_max.

Please let me know your feedback and/or feel free to review the upcoming PR PR.

sdasgup3 added a commit that referenced this issue May 10, 2023
## Summary 

The PR proposes the specification for quantized add op.

## A few details

At some point we
[decided](#1352 (comment))
to drop the introduction of the specification of this op mainly because
we were unsure about the fate of
#1406.
 
Please have a look at my revised proposal on
#1406 and let me know if I am
missing something. Otherwise, let us review this op and let me know your
feedback.

Side note: For those who are already aware of the context of prior
introduction of this op, please note that the current proposal is almost
same as before except that it does not have any additional constraint
imposed by the op's semantics on `storage_min` or `storage_max`.
@jingpu
Copy link

jingpu commented Jun 8, 2023

Here are some pointers regarding the "narrow_range" INT8 feature in TFLite.

Op specs: https://www.tensorflow.org/lite/performance/quantization_spec

CONV_2D
  Input 0:
    data_type  : int8
    range      : [-128, 127]
    granularity: per-tensor
  Input 1 (Weight):
    data_type  : int8
    range      : [-127, 127]
    granularity: per-axis (dim = 0)
    restriction: zero_point = 0

Essentially, the symmetric INT8 quantization is designed not to use the value 128 (I heard it was for leveraging some fast ARM instruction to implement the INT8 kernels).

The quantizer and TFL dialect MLIR importer also incorporate this feature.
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/mlir/lite/quantization/quantization_driver.cc#L390-L393
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/mlir/lite/flatbuffer_import.cc#L173

@sdasgup3
Copy link
Member Author

sdasgup3 commented Jun 12, 2023

@jingpu Thanks for your comment and explaining on the narrow range on INT8 quantization feature in TFLite.

I note that the current spec expects default values of min_value(storage_type) and max_value(storage_type) for storage_min and storage_max resp., if they are not explicitly provided.

Having said that, I am wondering, as the default value of storage_type like [-128,127] does not work in this case, how about the QuantizedType to explicitly encode [-127, 127] for the weights.

Please let me know your feedback!

@jingpu
Copy link

jingpu commented Jun 12, 2023

Sorry. I am a bit confused by what the proposal is. During the meeting last week, I thought the proposal was to drop storage_min and storage_max from the StableHLO quantize type, and use [min_value(storage_type), max_value(storage_type)] as the range. Could you clarify it?

@sdasgup3
Copy link
Member Author

Hi @jingpu
Sorry for the confusion :(. Actually, my goal in the meeting is to brainstorm on

  1. If storage_min and storage_max should have their values constraint for each op, as the case in TFLite spec, Or
  2. To follow what we currently have, which is, min_value(storage_type) and max_value(storage_type) as default value, if storage_min and storage_max, resp., are absent. If they are present, then the following constraint need to be satisfied.
min_value(storage_type) <= storage_min < storage_max <= max_value(storage_type).

I agree with you that if the allowed values are only min_value(storage_type) and max_value(storage_type), then it would be a problem expressing the use-case that you provided.

I think the description of the issue is misleading. I have updated the issue description to avoid any confusion.

@jingpu
Copy link

jingpu commented Jun 13, 2023

Thanks for the clarification. I don't have a strong preference between the two because I don't depend on the TFLite narrow_range feature in my work and don't have the full context about it either.

@sdasgup3
Copy link
Member Author

sdasgup3 commented Apr 9, 2024

The current spec allows the storage_min/storage_max values to be provided explicitly to cover cases like exciting use cases (like narrow_range mentioned above). With that let us close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Status: Done
Development

No branches or pull requests

3 participants