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

[trunc/quant_avg_pool] Update Trunc and QuantAveragePool to match how Brevitas Ops work #170

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nickfraser
Copy link

@nickfraser nickfraser commented Feb 11, 2025

Brevitas used to have some weird behaviour W.R.T. how average pool and trunc used to work (both separately and together). The original behaviour and a new proposal is detailed here. The purpose of this PR is to make changes on the QONNX side which match the new proposal.

TODO:

  • Update Trunc behaviour to match the new behaviour in Brevitas
  • Update the trunc_op spec to match the new functionality
  • Update QuantAveragePool2D to match the new behaviour in Brevitas - TODO in a separate PR

@maltanar
Copy link
Collaborator

maltanar commented Mar 7, 2025

Thanks @nickfraser ! For matching the new (fixed) behavior in Xilinx/brevitas#1042 the changes seem appropriate. A few thoughts on how to proceed:

  • The human-readable spec in https://github.com/fastmachinelearning/qonnx/blob/main/docs/qonnx-custom-ops/trunc_op.md should be updated to match the new 'Trunc` op definition and attributes
  • QuantAveragePool2D in QONNX should be updated correspondingly, or alternatively, eliminated entirely (e.g. FINN directly pattern-matches AveragePool -> Mul -> TruncQuant to its hardware op)
  • Backwards compatibility: Several pre-exported QONNX models in our ecosystem, e.g. MobileNet-v1 in the QONNX model zoo and in finn-examples use the previous behavior. Two ways in which we could resolve this:
    • Deprecate the old behavior entirely and only support the new behavior. All exported QONNX files in finn-examples and QONNX model zoo that use TruncQuant are updated to include the new attributes, and correspondingly for FINN/finn-examples flows.
    • Introduce versioning of the QONNX ops. Support both the old and the new behavior, depending on the version number. Since no versioning exists from before, we can introduce it and treat the presence of the new attributes (narrow, signed, output_scale) for version number inference in models where it is not specified.

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 this pull request may close these issues.

2 participants