Skip to content

gateware: Fixed-point arithmetic fixes and other improvements - #1804

Open
mndza wants to merge 12 commits into
greatscottgadgets:mainfrom
mndza:gateware-fixes
Open

gateware: Fixed-point arithmetic fixes and other improvements#1804
mndza wants to merge 12 commits into
greatscottgadgets:mainfrom
mndza:gateware-fixes

Conversation

@mndza

@mndza mndza commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This PR is the result of updating the fixed-point library with the code referenced in #1796 (comment). Adapting the gateware to the new semantics exposed several bugs and required additional optimizations to fit the designs within FPGA fabric.

Changes:

  • Updated the fixed point dependency to align with [WIP] Implement RFC 41: lib.fixed amaranth-lang/amaranth#1578. Minor API differences were resolved. The only modification is the addition of a rounding method based on our convergent_round implementation.
  • Added convergent_round fixes from fpga: Fixes to fixed-point arithmetic #1796 (thanks to @mildsunrise)
  • Modified convergent_round to avoid extending the bit width by default. An optional clip parameter can be set to False to disable clipping.
  • FIR Filters now have explicit rounding and saturation stages, if needed. This was not properly handled before and some overflows might occur.
  • Refactored the multiple constant multiplication (MCM) implementation. Future work may increase pipeline depth to reuse additional intermediate results.
  • fir_mac16: Replaced the skid buffer in DSP blocks with a proper clock enable signal, improving behavior in ext_precision_txgateware.
  • CIC: General improvements, added an optimization for the M=2 upsampling case.
  • Improved complex mixer architecture.
  • Add tests for fixed point library, complex mixer and refactor tests for FIR and CIC.

@mndza
mndza marked this pull request as draft July 28, 2026 12:50
@mndza
mndza force-pushed the gateware-fixes branch 2 times, most recently from 43541f1 to 172feae Compare July 29, 2026 12:47
@mndza
mndza marked this pull request as ready for review July 29, 2026 13:01
@mndza
mndza marked this pull request as draft July 29, 2026 14:53
convergent_round returns a value that is one bit longer than the non-fractional part of the passed value. it makes sense to pass a value that is exactly `discarded_bits` long IF it is unsigned. make convergent_round tolerate this edge case rather than fail, but make sure it still fails if `value` has less than `discarded_bits` non-sign bits.

more practically, this fixes an error when assigning a fixed.Value of i_width=0 and f_width > 0 to another with f_width=0
when convergent_round is passed a signed value, not only does it return an unsigned value, the *contents* are wrong since the addition is done with the wrong signedness (since slicing drops signedness)
@mndza
mndza requested a review from mossmann August 12, 2026 08:22
@mndza
mndza marked this pull request as ready for review August 12, 2026 08:22
@mossmann mossmann self-assigned this Aug 12, 2026
@mossmann
mossmann requested a review from miek August 12, 2026 16:15

@miek miek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, though it hits the limits when I try to build it to test it out. I'm not sure if it's still in progress or needs a particular toolchain setup?

@miek miek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that this includes the updated binary, so I've done some testing with that and it all looks good to me.

@mndza

mndza commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Looks good to me, though it hits the limits when I try to build it to test it out. I'm not sure if it's still in progress or needs a particular toolchain setup?

Yes, that’s an issue we’ve consistently run into. I’ll look into how to make the builds reproducible. More relaxed timing requirements would probably make this less of a problem, but some of the gateware already uses most of the available logic resources, making routing difficult.

I missed that this includes the updated binary, so I've done some testing with that and it all looks good to me.

Great 👍

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.

4 participants