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

Mismatch between specification and tests for quantized types #2539

Closed
jtristan opened this issue Sep 12, 2024 · 3 comments · Fixed by #2541
Closed

Mismatch between specification and tests for quantized types #2539

jtristan opened this issue Sep 12, 2024 · 3 comments · Fixed by #2541
Assignees

Comments

@jtristan
Copy link

What happened?

The tests in stablehlo/stablehlo/tests/interpret and stablehlo/stablehlo/testdata use a different syntax than what is described in the specification. Here is a grammar that describes the actual syntax of quantized types in those tests:

QuantizedTensorType ::= 'tensor' '<' Shape QuantizedTensorElementType '>'
QuantizedTensorElementType ::= '!quant.uniform' '<'
                  QuantizationStorageType
                  ['<' QuantizationStorageMin ':' QuantizationStorageMax '>']
                  ':' QuantizationExpressedType
                  [':' QuantizationDimension]
                  ',' QuantizationParameters '>'
QuantizationStorageType ::= IntegerType
QuantizationStorageMin ::= **IntegerLiteral**
QuantizationStorageMax ::= **IntegerLiteral**
QuantizationExpressedType ::= FloatType
QuantizationDimension ::= **IntegerLiteral**
QuantizationParameters ::= QuantizationParameter
                         | '{' QuantizationParameter {',' QuantizationParameter} '}'
QuantizationParameter ::= QuantizationScale **[ ':' QuantizationZeroPoint ]**
QuantizationScale ::= **FloatLiteral**
QuantizationZeroPoint ::= **IntegerLiteral**

where ** and ** emphasize where the differences with the specification are

Steps to reproduce your issue

No response

Version information

No response

@sdasgup3 sdasgup3 self-assigned this Sep 12, 2024
@sdasgup3
Copy link
Member

sdasgup3 commented Sep 12, 2024

Dear @jtristan
I am assuming that you refer to the absence of the non terminals highlighted above. Please note that QuantizationStorageMin, QuantizationStorageMax are optional and If not present inferred from the storage type.
QuantizationDimension is relevant only for per-axis quantization and absent for per-tensor.
QuantizationDimension is optional if the zero-point is zero.

Please let me know if I am missing any discrepancies.

@jtristan
Copy link
Author

No, the discrepancies are that the spec defines the grammar of QuantizationStorageMin, QuantizationStorageMax, QuantizationDimension , QuantizationScale , and QuantizationZeroPoint respectively as IntegerConstant , IntegerConstant , IntegerConstant , FloatConstant , IntegerConstant but that it should be IntegerLiteral, IntegerLiteral, IntegerLiteral, FloatLiteral, IntegerLiteral instead. Finally, the spec doesn't capture that QuantizationZeroPoint is optional.

@sdasgup3
Copy link
Member

Thanks for pointing those out. Will fix is asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants