You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR is focused on resolving issues reported as well as TODOs documented on the arithmetic library portion of ROHD-HCL. The biggest fixes are around rounding and conversions.
Related Issue(s)
Features
Added IEEE rounding modes across floating-point values and logic operations. FixesAdd all rounding modes #191.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Square-root handling, directed conversion overflow, MAC definition naming, and metadata consistency have unresolved correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
lib/src/arithmetic/fixed_to_float.dart:46
The new rounding mode affects significand rounding, but overflow still unconditionally emits infinity in the expoMoreThanMax branch. Directed/toward-zero modes must instead return the largest finite value when the rounding direction is away from infinity (for example, +15 converted to E3M2 with roundTowardsZero should become +14, not +infinity).
Underflow is hard-wired low even when the newly supported square root produces an inexact subnormal result. This contradicts FloatingPointStatus.underflow's “tiny result was inexact after rounding” contract; the smallest E3M5 subnormal exercised by the new test is one such case.
The PR's backwards-compatibility section says FixedPointValue now defaults to signed: true, but this constructor deliberately retains false (as does the deprecated populator), while the changelog describes that preservation. Align the PR description with this compatibility strategy, or change the implementation if the stated breaking default is intended.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Mixed-signed MAC arithmetic, conversion overflow rounding, and square-root underflow status contain unresolved correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
lib/src/arithmetic/float_to_fixed.dart:173
Overflow is calculated from the unrounded magnitude and never includes rounder.doRound. For signed Q2.1, E4M4 input 3.625 with round-nearest-even truncates to raw 7 (apparently fitting), then rounds to raw 8, wraps to -4, and leaves overflow false. Compute overflow from the rounded magnitude so rounding carry across the positive limit is reported.
The PR's backwards-compatibility section says FixedPointValue and its factory now default to signed: true, but this public factory still defaults to false and forwards that value. This also leaves the API mismatch from #249 in place for existing callers; either implement the documented breaking default or update the PR description to state that only the new withSignedness API uses the signed default.
commonWidth + 1 is insufficient when exactly one operand is signed. For example, the new test computes unsigned 49 + signed 15: the exact result is 64, but in 7 bits bit 6 is mistaken for a sign bit and widening produces 192. Reserve two bits beyond the widest operand so every mixed-signed sum remains representable before fitAccumulateWidth.
The rounding branch is selected by stored mantissa width, but explicit-J formats spend one stored bit on the J bit. An implicit M4 source converted to an explicit M4 destination loses one fraction bit, yet this condition skips the rounder, so all rounding modes behave like truncation (for example, positive 1.0625 should become 1.125 when rounding toward +infinity). Compare effective fraction widths and adjust the retained windows accordingly.
For explicit-J-bit formats this sets only the J bit, leaving every fraction bit zero. The resulting value is classified as infinity rather than NaN (isNaN checks the fractional portion), so FloatingPoint.nan(explicitJBit: true, ...) is broken. Build the constant through the value populator so the J bit and quiet-NaN bit are both encoded correctly.
final mantissa =
Const(BigInt.one << (mantissaWidth - 1), width: mantissaWidth);
The PR description says the FixedPointValue factory now defaults to signed: true, but this public factory still defaults to false (as does the deprecated populator). The code and changelog preserve backward compatibility, so either update the PR compatibility statement or change these defaults to match the stated breaking change.
For E4M3, expoMoreThanMax is still computed against encoded exponent 14 even though exponent 15 contains finite values. This branch therefore maps representable values such as 256 to the largest finite value (448). Derive the overflow limit from largestFinite.exponent rather than the generic infinity-reserving limit.
Biased exponent zero is also a subnormal result, but this condition only detects negative roundedExponent. For example, sqrt(E3M5 raw 0 000 00010) is exactly raw 0 000 10000; here roundedExponent is zero, so the current normal path emits exponent zero with the hidden bit discarded (zero instead of 0.125). Include zero in the subnormal condition.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description & Motivation
This PR is focused on resolving issues reported as well as TODOs documented on the arithmetic library portion of ROHD-HCL. The biggest fixes are around rounding and conversions.
Related Issue(s)
Features
Fixes Add all rounding modes #191.
FloatingPointValue/FixedPointValueconversions. Fixes Add converters betweeFloatingPointValueandFixedPointValue#112.toLogic()conversion for fixed- and floating-point values. Fixes Add an easy way for arithmeticValues to convert toConstversions of corresponding signals #200.Fixes
zero multiplication. Fixes FloatingPointMultiplier to narrower FP type #194.
boundaries. Fixes RoundRNE is not rounding to even, both logic and value side #190.
FloatingPointConverterconversions and added exhaustiveimplicit-j-bit subnormal coverage. Fixes FloatingPointConverter fails on narrowing mantissa with explicitJBit #241.
FloatingPointValue.ulp()for subnormals. Fixes FloatingPointValue unit of least precision (ulp) is incorrect for subnormals #206.FloatingPointValue.operator /exact and correctly rounded. Fixes MakeFloatingPointValueoperations scale beyonddouble#113.FloatToFixedexplicit-j-bit conversion, overflow handling, anddiscarded-bit sticky rounding.
boolean control widths.
API Cleanup and Verification
StaticOrRuntimeControl<T>as the general static-or-runtime controlabstraction; deprecated
BooleanConfigandRuntimeConfig.multiply-accumulate coverage.
Testing
All existing tests pass. Several new tests added to cover bugs reported and fixed.
Backwards-compatibility
FixedPointValue, its factory, and its populator now default tosigned: true, matchingFixedPoint. Passsigned: falseto retain theprevious unsigned behavior. Fixes FixedPoint and FixedPointValue have different default values for signed #249.
Documentation
Yes. Relevant documentation was updated to match the new functionality introduced (e.g.,
StaticOrRuntimeControl.