Skip to content

Conversation

@fabiomadge
Copy link
Contributor

@fabiomadge fabiomadge commented Jul 24, 2025

This PR fixes a critical bug in BigFloat.FromInt() where it improperly used the string constructor, causing arithmetic failures. The fix introduces IEEE 754-compliant conversions, removes redundant fields, and adds 177 comprehensive tests to ensure correctness.

The Bug: FromInt Misused the String Constructor

The value field in BigFloat was designed to store only special values ("NaN", "+oo", "-oo"), with empty string ("") indicating a normal numeric value. However, FromInt was incorrectly implemented:

// OLD BROKEN CODE:
public static BigFloat FromInt(int v) {
    return new BigFloat(v.ToString(), 24, 8);  // e.g., "42"
}

This stored regular integers as strings in the value field (e.g., "42"), breaking the invariant that value should only contain special values or be empty. This caused arithmetic operations to fail because:

  1. The string constructor didn't initialize the IEEE 754 fields (significand, exponent)
  2. Arithmetic operations' special value handling would see value != "" and try to handle "42" as a special value
  3. The actual numeric representation was never properly initialized

The Fix

  • Changed FromInt to use proper IEEE 754 conversion via FromRational
  • Removed the problematic string constructor that allowed non-special values

Other Improvements

  1. Parameter Order Fix: Corrected FromBigInt(value, exponentSize, significandSize) to have consistent parameter order
  2. Removed Dual Representation: Eliminated the value field entirely in favor of proper IEEE 754 representation for all values
  3. IEEE 754 compliant arithmetic in all cases
  4. Comprehensive Tests: Added 177 tests covering integer conversion, arithmetic, and edge cases

This fix ensures that integer-to-float conversions work correctly and participate properly in arithmetic operations.

shazqadeer
shazqadeer previously approved these changes Jul 24, 2025
Copy link
Contributor

@shazqadeer shazqadeer left a comment

Choose a reason for hiding this comment

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

I am approving the PR with low confidence since I am not an expert in floating point. Perhaps @zvonimir, who is familiar with both floating point and Boogie, can also take a look.

@shazqadeer
Copy link
Contributor

@fabiomadge : Thanks for the PR. Please look into why some of the regression tests are failing.

@fabiomadge
Copy link
Contributor Author

On it, thanks!

@fabiomadge fabiomadge marked this pull request as draft July 24, 2025 21:22
@fabiomadge fabiomadge marked this pull request as ready for review July 25, 2025 01:21
@fabiomadge fabiomadge marked this pull request as draft July 25, 2025 21:41
- Fix overly aggressive subnormal overflow detection that was incorrectly
  promoting valid subnormals to normal numbers
- Add proper handling for 0.5 × smallest subnormal to round to zero per
  IEEE 754 round-to-nearest-even rules
- Fix complete underflow path for values below minimum subnormal exponent
- Add comprehensive test coverage for subnormal edge cases

Also adds many unit tests.
@fabiomadge fabiomadge marked this pull request as ready for review July 29, 2025 20:34
@fabiomadge
Copy link
Contributor Author

@shazqadeer I spoke with @zvonimir and he won't immediately get to it. If you're ok with it, I'd like to merge now and revisit this, in case we end up finding any issues.

shazqadeer
shazqadeer previously approved these changes Jul 29, 2025
@shazqadeer
Copy link
Contributor

Approved. Thanks for the PR.

@fabiomadge
Copy link
Contributor Author

I don't have a compelling explanation for what went wrong, but installing .NET 6 on the runner solved the build issue we encountered after merging the README.md changes.

@shazqadeer
Copy link
Contributor

I have seen the CI to be flaky in the past occasionally. A retry can often do the job. Please retry after reverting your change to the runner. I would like to see if that fixes the problem.

@fabiomadge
Copy link
Contributor Author

It took three retries, but as finally worked.

@shazqadeer shazqadeer merged commit 892f475 into master Jul 30, 2025
5 of 13 checks passed
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.

3 participants