-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix ILogB for Half and BFloat16 subnormal values #123294
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
Fix ILogB for Half and BFloat16 subnormal values #123294
Conversation
BitOperations.LeadingZeroCount has no ushort overload, so the TrailingSignificand is implicitly widened to uint, adding 16 extra leading zeros. Subtract 16 to correct for this widening. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…byte Instead of adjusting the value at the call site, add internal overloads of BitOperations.LeadingZeroCount for ushort and byte types that handle the widening adjustment internally. Update UInt16, Int16, Char, Byte, and SByte LeadingZeroCount methods to use these new internal overloads. Co-authored-by: stephentoub <2642209+stephentoub@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 ILogB for Half and BFloat16 where subnormal values returned incorrect results due to implicit type widening. When BitOperations.LeadingZeroCount was called with a ushort argument (the type of TrailingSignificand), it was implicitly widened to uint, adding 16 extra leading zeros to the count.
Changes:
- Added internal
LeadingZeroCountoverloads forushortandbyteinBitOperationsthat handle the bit-width adjustment - Refactored
LeadingZeroCountinUInt16,Int16,Byte,SByte, andCharto use the new internal overloads, removing redundant manual adjustments - Added comprehensive tests for
ILogBcovering normal, special, and subnormal values for bothHalfandBFloat16
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| BitOperations.cs | Adds internal LeadingZeroCount overloads for ushort and byte that correctly adjust for bit width when widening to uint |
| UInt16.cs | Removes manual -16 adjustment, now uses internal LeadingZeroCount(ushort) overload |
| Int16.cs | Removes manual -16 adjustment, now uses internal LeadingZeroCount(ushort) overload |
| Byte.cs | Removes manual -24 adjustment, now uses internal LeadingZeroCount(byte) overload |
| SByte.cs | Removes manual -24 adjustment, now uses internal LeadingZeroCount(byte) overload |
| Char.cs | Removes manual -16 adjustment, now uses internal LeadingZeroCount(ushort) overload |
| HalfTests.cs | Adds tests for ILogB including normal values, special values, and two subnormal test cases |
| BFloat16Tests.cs | Adds tests for ILogB including normal values, special values, and two subnormal test cases |
Per reviewer feedback, use the existing ushort.LeadingZeroCount method which already handles the bit-width adjustment, instead of adding new internal overloads to BitOperations. Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
src/libraries/System.Private.CoreLib/src/System/Numerics/BFloat16.cs
Outdated
Show resolved
Hide resolved
Merged latest changes from main and updated BFloat16.ILogB to use ushort.LeadingZeroCount consistently with Half.ILogB, since TrailingSignificand is ushort for both types. Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
Undoes the previous merge commit that was not done correctly. Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
…ormula - Changed TrailingSignificand property from ushort to byte - Changed ExtractTrailingSignificandFromBits to return byte - Changed MinTrailingSignificand and MaxTrailingSignificand constants to byte - Updated IsPow2 to use byte.PopCount - Updated ILogB to use byte.LeadingZeroCount with adjusted formula Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
ILogBforHalfandBFloat16returns incorrect values for subnormal numbers becauseBitOperations.LeadingZeroCountonly has overloads foruint/ulong/nuint, so theushortfromTrailingSignificandis implicitly widened touint, adding 16 extra leading zerosHalf.ILogBto useushort.LeadingZeroCountwhich handles the bit-width adjustmentBFloat16.ILogBto usebyte.LeadingZeroCountafter changingTrailingSignificandto returnbyteHalf.ILogBandBFloat16.ILogBwith normal and subnormal valuesSecurity Summary
No security vulnerabilities were discovered or introduced by these changes.
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.