Add Decimal32, Decimal64, Decimal128 #100729
Conversation
|
Note regarding the |
src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs
Outdated
Show resolved
Hide resolved
| static int IDecimalIeee754UnpackInfo<Decimal128, Int128, UInt128>.NumberDigitsPrecision => NumberDigitsPrecision; | ||
|
|
||
|
|
||
| private static Int128[] Int128Powers10 => |
There was a problem hiding this comment.
If Int128 is not optimized, this can be optimally stored as a ReadOnlySpan of Int32/Int64 like Number.BigInteger.Pow10BigNumTable
| public override int GetHashCode() | ||
| { | ||
| return _value.GetHashCode(); | ||
| } |
There was a problem hiding this comment.
+0 and -0 should have the same hash code. The easiest way can be stripping the sign bit.
I also remember that there can be same values with different representations.
There was a problem hiding this comment.
@huoyaoyuan Thanks for your review. However this PR targets only the first phase as following: #81376 (comment) so I haven't added NegativeZero and PositiveZero. I will update GetHashCode method in the next phase.
src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal128.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.cs
Outdated
Show resolved
Hide resolved
|
This is on my radar to take a look at; just noting it might be a bit delayed due to other priorities for the .NET 9 release. CC. @jeffhandley |
|
|
||
| if (number.DigitsCount + numberZeroDigits <= TDecimal.Precision) | ||
| { | ||
| byte* p = number.DigitsPtr + number.DigitsCount; |
There was a problem hiding this comment.
Similar case, we just want to use Span<byte> and the number.Digits.Slice(number.DigitsCount) instead
There was a problem hiding this comment.
(assuming its not rewritten to avoid using a string buffer entirely, which I think is possible and likely faster/better)
There was a problem hiding this comment.
Thanks, I have updated it.
| public bool Signed { get; } | ||
| public int Exponent { get; } | ||
| public TSignificand Significand { get; } |
There was a problem hiding this comment.
This and other places could likely be a bit more explicit, such as specifying whether this is the biased or unbiased exponent and which part of the significand it is (such as if its already undergone the BID encoding transforms)
There was a problem hiding this comment.
Thanks, I have updated the struct name and its property names to make it more readable
| where TDecimal : unmanaged, IDecimalIeee754ParseAndFormatInfo<TDecimal, TValue> | ||
| where TValue : unmanaged, IBinaryInteger<TValue> | ||
| { | ||
| bool signed = (value & TDecimal.SignMask) != TValue.Zero; |
There was a problem hiding this comment.
Looks like value is rather decimalBits, which would make it clearer it's not some arbitrary integer
There was a problem hiding this comment.
Thanks, I have updated it
| TValue significand; | ||
| int exponent; | ||
|
|
||
| if ((value & TDecimal.G0G1Mask) == TDecimal.G0G1Mask) |
There was a problem hiding this comment.
Given the binary string abcdmmmmefgt..., this is checking ab == 11 which indicates that we have exponent: cdmmmmef and significand: 100gt... instead of exponent: abcdmmmm and significand: 0efgt...
However, I don't see this handling abcd == 1111 which indicates it is an infinity or nan.
There was a problem hiding this comment.
Hi @tannergooding, this method is designed to encode the finite number case. I have updated the method name to reflect it. Infinite/rounding/normalize cases are handled outside of this method.
For NaN case, based on your suggestion(#81376 (comment)) I'm doing the first step, so I didn't include the NaN case handling in this PR. Just let me know if it should be included 😉
There was a problem hiding this comment.
NaN, negative zero, and infinity would all be considered part of the “basic handling” that allows this to be purely used as an interchange type.
The intent is to ensure that users can parse, format, and construct valid decimal values. Since those are all valid and core values, they need to be supported to ensure no behavioral breaking changes or bugs exist.
Effectively we want it to be a “minimal viable product” that could ship even if we didn’t have time to add support for all arithmetic operations
| if (current.Signed) | ||
| { | ||
| (current, other) = (other, current); | ||
| } |
There was a problem hiding this comment.
This could just be handled in the earlier branch as the else case, to avoid checking the sign twice
There was a problem hiding this comment.
It can also be done prior to unpacking, to save on some perf
There was a problem hiding this comment.
Thanks, I have updated it
| } | ||
| } | ||
|
|
||
| private static unsafe TValue NumberToDecimalIeee754Bits<TDecimal, TValue>(ref NumberBuffer number) |
There was a problem hiding this comment.
Similar case, we want to avoid unnecessary unsafe. The JIT should be able to do a generally good job with all the patterns we're using here
There was a problem hiding this comment.
Thanks, I have updated it
| } | ||
|
|
||
| significand = TDecimal.NumberToSignificand(ref number, number.DigitsCount); | ||
| return DecimalIeee754BinaryEncoding<TDecimal, TValue>(number.IsNegative, significand, exponent); |
There was a problem hiding this comment.
this return is duplicative of the return below
The function could also be refactored slightly to make it a bit shorter and simpler.
if ((number.DigitsCOunt <= TDecimal.Precision) || !DecimalIeee754Rounding<TDecimal, TValue>(ref number, TDecimal.Precision, out TValue significand, out int exponent))
{
int positiveExponent = (Math.Max(0, number.Scale));
int integerDigitsPresent = Math.Min(positiveExponent, number.DigitsCount);
int fractionalDigitsPresent = number.DigitsCount - integerDigitsPresent;
exponent = number.Scale - integerDigitsPresent - fractionalDigitsPresent;
if (exponent < TDecimal.MinExponent)
{
return SmallExponentRounding(ref number, exponent);
}
significand = TDecimal.NumberToSignificand(ref number, number.DigitsCount);
}
else if (exponent > TDecimal.MaxExponent)
{
return number.IsNegative ? TDecimal.NegativeInfinity : TDecimal.PositiveInfinity;
}
return DecimalIeee754BinaryEncoding<TDecimal, TValue>(number.IsNegative, significand, exponent);| static TValue SmallExponentRounding(ref NumberBuffer number, int exponent) | ||
| { | ||
| Debug.Assert(exponent < TDecimal.MinExponent); | ||
| int numberDigitsRemove = (TDecimal.MinExponent - exponent); | ||
| if (numberDigitsRemove < number.DigitsCount) | ||
| { | ||
| int numberDigitsRemain = number.DigitsCount - numberDigitsRemove; | ||
| DecimalIeee754Rounding<TDecimal, TValue>(ref number, numberDigitsRemain, out TValue significand, out exponent); | ||
| return DecimalIeee754BinaryEncoding<TDecimal, TValue>(number.IsNegative, significand, exponent); | ||
| } | ||
| else | ||
| { | ||
| return TDecimal.Zero; | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure it's worth extracting this to a local method. It's likely to be inlined already plus results in a bit simpler code, with less asserts, if just declare it inline manually
There was a problem hiding this comment.
Thanks, I have updated it to be inlined.
| if (exponent > TDecimal.MaxExponent) | ||
| { | ||
| return number.IsNegative ? TDecimal.NegativeInfinity : TDecimal.PositiveInfinity; | ||
| } |
There was a problem hiding this comment.
This path likely deserves a comment (and maybe explicit test) how it handles TDecimal.MaxValue + (Delta / 2) vs TDecimal.MaxValue + ((Delta / 2) - 1)
i.e. float.MaxValue is 340282346638528859811704183484516925440 and the delta between representable values at that range is 20282409603651670423947251286016. So 340282346638528859811704183484516925440 + (20282409603651670423947251286016 / 2) is 340282356779733661637539395458142568448
You will then find that 340282356779733661637539395458142568448 rounds to +Infinity, while 340282356779733661637539395458142568447 rounds to float.MaxValue -- i.e. values larger than MaxValue can still round down if they are "nearer"
| } | ||
| } | ||
|
|
||
| private static unsafe TValue DecimalIeee754BinaryEncoding<TDecimal, TValue>(bool signed, TValue significand, int exponent) |
There was a problem hiding this comment.
nit: exponent and significand are swapped compared to the typical order of such APIs
There was a problem hiding this comment.
These types of APIs, even though they are private/internal, likely also benefit from XML doc comments to be explicit that that significand is the "full" (non-trailing) significand and exponent is the unbiased exponent.
The terms get interchanged a lot and so being explicit can help ensure future readers understand the intent.
| byte[] buffer = ArrayPool<byte>.Shared.Rent(TDecimal.BufferLength); | ||
| try | ||
| { | ||
| fixed (byte* pDigits = buffer) |
There was a problem hiding this comment.
more cases that are using unnecessary unsafe. NumberBuffer takes a span
| char* stackPtr = stackalloc char[CharStackBufferSize]; | ||
| var vlb = new ValueListBuilder<char>(new Span<char>(stackPtr, CharStackBufferSize)); |
There was a problem hiding this comment.
this can directly use stackalloc in the ValueListBuilder constructor as well, since the language will convert it to span implicitly
There was a problem hiding this comment.
Hi @tannergooding , I follow your instruction, but it throws this error,
do you have another suggestion, or we just keep the old one.
There was a problem hiding this comment.
Yeah, unfortunately there's actually a missing language feature here due to how we pass the data to NumberToString. We need ref scoped SomeRefStruct to do this one in particular.
Other callouts may not need that feature, just this one in particular does.
| finally | ||
| { | ||
| ArrayPool<byte>.Shared.Return(buffer); | ||
| } |
There was a problem hiding this comment.
We don't typically put the return in a finally block. An exception being thrown is very unexpected and returning is not required for correctness
There was a problem hiding this comment.
Thanks, I have removed the finally block.
| where TValue : unmanaged, IBinaryInteger<TValue> | ||
| { | ||
| byte* buffer = number.DigitsPtr; | ||
| DecimalIeee754<TValue> unpackDecimal = value.Unpack(); |
There was a problem hiding this comment.
Unpack should likely not be an extension method.
| DecimalIeee754<TValue> unpackDecimal = value.Unpack(); | ||
| number.IsNegative = unpackDecimal.Signed; | ||
|
|
||
| byte* p = buffer + TDecimal.Precision; |
There was a problem hiding this comment.
More cases that should use Span instead. There's a number of them and I don't plan to comment every instance
| private static readonly UInt128 PositiveInfinityValue = new UInt128(upper: 0x7800_0000_0000_0000, lower: 0); | ||
| private static readonly UInt128 NegativeInfinityValue = new UInt128(upper: 0xf800_0000_0000_0000, lower: 0); | ||
| private static readonly UInt128 ZeroValue = new UInt128(0, 0); | ||
| private static readonly UInt128 NegativeZeroValue = new UInt128(0x8000_0000_0000_0000, 0); |
There was a problem hiding this comment.
Can this be a problem for codegen since UInt128 is not const?
There was a problem hiding this comment.
They should probably be properties instead
There was a problem hiding this comment.
Thanks, I have updated it.
| return Number.FormatDecimalIeee754<Decimal128, UInt128>(this, format, NumberFormatInfo.GetInstance(provider)); | ||
| } | ||
|
|
||
| private static UInt128[] UInt128Powers10 => |
There was a problem hiding this comment.
This can be a problem for constant embedding. long array can be encoded into executable and read by pairs.
Resolve #81376