-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix BFloat16.Significand to be byte-sized and correct TryWriteSignificand methods #123288
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
Conversation
- Changed bytesWritten from sizeof(uint) to sizeof(ushort) in TryWriteSignificandBigEndian - Changed bytesWritten from sizeof(uint) to sizeof(ushort) in TryWriteSignificandLittleEndian - Added comprehensive tests for both methods Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug in BFloat16.TryWriteSignificandBigEndian and TryWriteSignificandLittleEndian where these methods incorrectly reported writing 4 bytes (sizeof(uint)) instead of 2 bytes (sizeof(ushort)) for the significand.
Changes:
- Fixed
bytesWrittento reportsizeof(ushort)instead ofsizeof(uint)in both methods - Added comprehensive test coverage for both methods to verify correct byte counts and output values
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| BFloat16.cs | Corrected bytesWritten from sizeof(uint) to sizeof(ushort) in TryWriteSignificandBigEndian and TryWriteSignificandLittleEndian |
| BFloat16Tests.cs | Added comprehensive test methods TryWriteSignificandBigEndianTest and TryWriteSignificandLittleEndianTest with 11 test cases each |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Numerics/BFloat16Tests.cs
Outdated
Show resolved
Hide resolved
- Converted TryWriteSignificandBigEndianTest to [Theory] with [InlineData] attributes - Converted TryWriteSignificandLittleEndianTest to [Theory] with [InlineData] attributes - Separated empty destination tests into dedicated [Fact] methods - Removed duplicate test cases (same significand for ±0.0 and ±Epsilon) Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
|
@copilot the tests you added fail on CI. e.g. |
- Changed from [InlineData] with float values to [MemberData] with BFloat16 values - Use BFloat16.MinValue/MaxValue instead of float.MinValue/MaxValue conversions - This ensures test data matches the actual BFloat16 representation - Fixes CI test failures where float-to-BFloat16 conversions produced different values Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
... Fixed in commit b44d1de. The issue was using float literals that converted to different BFloat16 values than expected. Changed to use BFloat16 constants directly (e.g., |
|
@copilot use the collection literal syntax |
- Changed from yield return new object[] { ... } to collection literal [ ... ] syntax
- Applied to both TryWriteSignificandBigEndianTest_TestData and TryWriteSignificandLittleEndianTest_TestData
- Uses expression-bodied member syntax with => for cleaner code
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Done in commit 062a536. Updated both test data methods to use collection literal syntax |
|
I believe BFloat16 has 8-bits significand? Then many codes in |
thanks! What do you mean by many codes? |
|
This comment was marked as outdated.
This comment was marked as outdated.
I'm a bit confused, @tannergooding @huoyaoyuan can you clarify the behavior here. So TryWriteSignificand used to report 4 bytes were written (which is clearly a bug), but the Significant itself is defined as ushort, while it looks like it's actually should be |
|
|
|
I copied the logic from |
|
@copilot it turns out Significand has to be byte-sized, can you changes all the relevant bits in BFloat16 ? |
- Changed Significand property from ushort to byte (BFloat16 has 8-bit significand) - Updated GetSignificandByteCount to return sizeof(byte) instead of sizeof(ushort) - Updated TryWriteSignificandBigEndian to write 1 byte directly (no endianness needed) - Updated TryWriteSignificandLittleEndian to write 1 byte directly (no endianness needed) - Updated test data to expect 1 byte written instead of 2 bytes - Updated test destination buffers from [0, 0] to [0] Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Done in commit ca5fb35. Changed |
We generally want the "size written" to be a primitive size and otherwise a multiple of So we have:
Which means we want:
Edit: sorry, hit enter too soon and it posted. It's then worth noting that despite there being that many "stored" bits, that is not how many bits actually exist. There is an implicit significand bit that exists, so the actual amount is:
This could theoretically bump the limit over to the next size in some cases. It doesn't for any of these, however, so the fixes in this PR should be correct. |
Description
BFloat16.TryWriteSignificandBigEndianandTryWriteSignificandLittleEndianinitially reportedbytesWritten = 4(sizeof(uint)) instead of the correct value. Upon further investigation, it was discovered that BFloat16's significand should be byte-sized (8 bits: 7 trailing bits + 1 implicit bit), not ushort-sized (16 bits).Changes
BFloat16.cs:
Significandproperty fromushorttobyteto correctly represent the 8-bit significandGetSignificandByteCount()to returnsizeof(byte)(1) instead ofsizeof(ushort)(2)TryWriteSignificandBigEndianto write 1 byte directly (endianness is irrelevant for single byte)TryWriteSignificandLittleEndianto write 1 byte directly (endianness is irrelevant for single byte)BFloat16Tests.cs: Added test coverage for
TryWriteSignificandBigEndianandTryWriteSignificandLittleEndianusing[Theory]with[MemberData]to provide BFloat16 test values directly. Tests verify correct byte count (1 byte) and output values for various BFloat16 values (NegativeInfinity, MinValue, NaN, Epsilon, Zero, MaxValue, PositiveInfinity). Empty destination edge cases are tested in separate[Fact]methods. Test data methods use modern C# collection literal syntax[ ... ]for cleaner, more maintainable code.Technical Details: BFloat16 format has 1 sign bit + 8 exponent bits + 7 trailing significand bits. The
Significandproperty includes the implicit leading 1 bit for normal numbers, resulting in 8 bits total (1 byte). For a single byte value, endianness doesn't apply, so both BigEndian and LittleEndian methods write the byte directly todestination[0].Note: Tests use
[MemberData]instead of[InlineData]to pass BFloat16 objects directly, avoiding float-to-BFloat16 conversion issues that could produce different values than expected.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.