fpga: Fixes to fixed-point arithmetic - #1796
Conversation
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)
fixes a bug where fixed.Value.round() would return an incorrect shape when it drops precision
I'm not sure why raw() fixes signedness in one direction but not the other? should read the RFC more closely
sometimes we want to use cast() to infer integer width while still being explicit about the desired shape signedness. currently closest we can do is setting signedness on the value passed to cast() but this (1) is less convenient, (2) has different semantics re. assignability as it prevents fixed.Value from being the one to set signedness in raw()
the optimization for `__lshift__` when the argument is an integer is not correct in some cases, since Cat() drops signedness
the optimization for `__rshift__` when `other` is an integer is not correct: in some cases, it will return a malformed fixed.Value (its _target's width doesn't match the shape's width) fix this by extending the integer if `other` is too high, in a dual way to what we do in `__lshift__` (but here we need to extend on the other side)
in fir_mac16, the size of the output register was being restricted but not the size of the carry register in fir, the mistake is that mcm creates an output shape as minimal as possible depending on the term's value, so a right shift is needed rather than a cast (to extend the value if needed)
|
Drive-by comment as I noticed this while searching for something else. It looks like this implementation is derived from an (old?) revision of this PR: amaranth-lang/amaranth#1578
|
|
@vk2seb thanks for the info! will try to make sure my fixes are aligned with those upstream ones then... |
|
Thank you! This was very helpful. I’ve taken the opportunity to cherry-pick some of your commits and update the fixed-point implementation to the newer version referenced above. This appears to address most of the issues you identified: #1804 Regarding I'm still trying to meet timings for the standard gateware, but it should be ready soon. |
|
thanks! closing this then |
3 participants
Hello! While reviewing the code for the fixed-point types I found a flurry of, in some cases clear bugs, in others unintuitive behavior at least.
I'm aware this code was copied from somewhere else, I just haven't yet had time to check where each issue was introduced so I can send each patch to the most upstream place.
These are the issues I found, each one has an associated patch (I can squash them all into one if preferred):
convergent_round()returns incorrect result when argument is negative:convergent_roundseems to be written without support for signed values, so for example -13 (which is -3.25 in SQ2.2) rounds to 5 rather than -3. Note that doing.as_signed()on the result is not enough; the problem is that theretained + round_upaddition is performed with unsigned inputs, and thereforeretainedis not correctly extended and the output MSB is wrong.This of course affects
fixed.Value.round()and all of its usages. I believe current usages indspmanage not to hit this because they immediately assign the rounded value to a signal in a way that drops the faulty MSB and signedness.convergent_round()fails whenlen(value) == discarded_bits: while this is a more questionable edge case, it means that under very specific argumentsfixed.Value.eq()fails (namely when self's f_width == 0 and other is unsigned, i_width == 0 and f_width > 0). I believe it makes perfect sense forconvergent_roundto handle this case, so I fixed it there.fixed.Value.round()returns malformed value: when dropping precision, afixed.Valueis returned whose innerhdl.Valuedoes not have a width matchingas_shape().width. This is because rounding produces an extra carry bit, but the code fails to reflect this in the shape by increasingi_width.fixed.Value.__lshift__()sometimes returns incorrect result: the optimization for whenotheris anintis not correct, because in some cases it returns a different value than what would be returned ifotherwere wrapped inConst. This is becauseCatdrops the signedness, thusfixed.Value.cast()reinterprets the value as if it were unsigned. For example,fixed.Const(-3.25) << 3returns 38, whilefixed.Const(-3.25) << Const(3)returns -26 as expected.fixed.Value.__rshift__()sometimes returns malformed value: the optimization for whenotheris anintis not correct, because in some cases it returns afixed.Valuewhose innerhdl.Valuedoes not have a width that matchesas_shape().width. This malformed value can later fail to round, as the raw value has less bits than expected.fixed.Value.raw()does not preserve signedness for unsigned values: it's less clear this is a bug, but I believeraw()should always return anhdl.Valueof the signedness specified byshape.signed, and it currently fails to do that when_targetis signed butshapeis not. Plus it adds theas_signed()wrapper even when it's not needed, affecting readability and assignability.Two of the issues above were related to malformed values, so I've also added an assertion in
__init__to catch mistakes like those in the future. This revealed two other creations of malformed values indspcode which were easy to fix.