Skip to content

Double to Decimal Conversion Refactor (WIP) #70602

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.

Sign up for GitHub

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

Closed
wants to merge 32 commits into from
Closed

Double to Decimal Conversion Refactor (WIP) #70602

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
391f805
WIP: decimal conversion bug
dakersnar May 3, 2022
a6dc156
Merge branch 'main' of https://github.com/dotnet/runtime into fix-68042
dakersnar May 3, 2022
edc3f67
Merge branch 'main' of https://github.com/dotnet/runtime into fix-68042
dakersnar May 3, 2022
cccae84
Merge branch 'main' of https://github.com/dotnet/runtime into fix-68042
dakersnar May 3, 2022
5a19f23
Merge branch 'fix-68042' of https://github.com/dakersnar/runtime into…
dakersnar May 9, 2022
b0721b8
Split up Dragon4 into two parts in order to reuse the first half later.
dakersnar Jun 3, 2022
d213837
Slight refactor to remove unneccesary calculation from the helper fun…
dakersnar Jun 3, 2022
728efa1
WIP: Decimal.DecCalc
dakersnar Jun 4, 2022
31820e3
Merge branch 'main' of https://github.com/dotnet/runtime into fix-68042
dakersnar Jun 4, 2022
1df3d17
WIP: Number.Dragon4.cs
dakersnar Jun 4, 2022
246483c
WIP: DecimalTests.cs
dakersnar Jun 4, 2022
40fcdd5
Initial unit test now passing
dakersnar Jun 10, 2022
7e6cd93
Updated decimal unit test file
dakersnar Jun 10, 2022
e77161f
WIP: Current state of this fix
dakersnar Jun 11, 2022
29bac4b
Remove unneeded include
dakersnar Jul 5, 2022
94b874b
Possible working solution
dakersnar Jul 6, 2022
6525508
Fix naming, fix typo of tests
dakersnar Jul 6, 2022
895b413
Adjust comments, tweak style, canonicalize result
dakersnar Jul 7, 2022
5d9edfa
Update unit test data to be more accurate for double and float
dakersnar Jul 11, 2022
e6dbd62
Fix edge cases
dakersnar Jul 11, 2022
2d191a1
Fix precision of returned zeros
dakersnar Jul 11, 2022
47ac650
Fix expected values in Decimal's Generic Math tests
dakersnar Jul 11, 2022
6db7625
Fix test data for InsertDecimal and AppendDecimal tests
dakersnar Jul 11, 2022
1d38279
Fix rounding issues
dakersnar Jul 12, 2022
1c3556d
Slight improvement for decimal->double conversion
dakersnar Jul 12, 2022
ceccdb5
Fix rounding
dakersnar Jul 12, 2022
2030e68
Normalize test data
dakersnar Jul 12, 2022
f1d0451
Revert "Slight improvement for decimal->double conversion"
dakersnar Jul 13, 2022
8a28c84
Remove accidental push to DecCalc
dakersnar Jul 13, 2022
6a7266c
Fix Number.BigInteger DivRem Bug
dakersnar Jul 13, 2022
a62767d
Remove round trip tests temporarily until decimal to double conversio…
dakersnar Jul 13, 2022
23b71de
Fix comment typos
dakersnar Jul 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 18 additions & 149 deletions src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Internal;
using System.Diagnostics;
using System.Numerics;
using System.Runtime.CompilerServices;
Expand Down Expand Up @@ -1687,173 +1688,41 @@ internal static void VarDecFromR4(float input, out DecCalc result)
/// <summary>
/// Convert double to Decimal
/// </summary>

internal static void VarDecFromR8(double input, out DecCalc result)
{
result = default;

// The most we can scale by is 10^28, which is just slightly more
// than 2^93. So a float with an exponent of -94 could just
// The smallest non-zero decimal we can represent is 10^-28, which is just slightly more
// than 2^-93. So a float with an exponent of -94 could just
// barely reach 0.5, but smaller exponents will always round to zero.
// This is a shortcut to catch doubles that are too small efficiently.
//
const uint DBLBIAS = 1022;
Copy link
Contributor Author

@dakersnar dakersnar Jun 11, 2022

Choose a reason for hiding this comment

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

As mentioned, this is conceptually incorrect, as possibly even logically incorrect

int exp = (int)(GetExponent(input) - DBLBIAS);
if (exp < -94)
return; // result should be zeroed out

if (exp > 96)
Number.ThrowOverflowException(TypeCode.Decimal);

uint flags = 0;
if (input < 0)
{
input = -input;
flags = SignMask;
}

// Round the input to a 15-digit integer. The R8 format has
// only 15 digits of precision, and we want to keep garbage digits
// out of the Decimal were making.
Comment on lines -1713 to -1715
Copy link
Contributor Author

@dakersnar dakersnar Jun 11, 2022

Choose a reason for hiding this comment

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

As mentioned, this is incorrect, and the cause of the original bug

//
// Calculate max power of 10 input value could have by multiplying
// the exponent by log10(2). Using scaled integer multiplcation,
// log10(2) * 2 ^ 16 = .30103 * 65536 = 19728.3.
//
double dbl = input;
int power = 14 - ((exp * 19728) >> 16);
// power is between -14 and 43

if (power >= 0)
if (input.Exponent < -94)
Copy link
Contributor Author

@dakersnar dakersnar Jun 11, 2022

Choose a reason for hiding this comment

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

I'm using the Exponent exposed on double now. I think this is more correct than the original comparison.

{
// We have less than 15 digits, scale input up.
//
if (power > DEC_SCALE_MAX)
power = DEC_SCALE_MAX;

dbl *= s_doublePowers10[power];
}
else
{
if (power != -1 || dbl >= 1E15)
dbl /= s_doublePowers10[-power];
else
power = 0; // didn't scale it
}

Debug.Assert(dbl < 1E15);
if (dbl < 1E14 && power < DEC_SCALE_MAX)
{
dbl *= 10;
power++;
Debug.Assert(dbl >= 1E14);
return; // result should be zeroed out
}

// Round to int64
// The smallest double with an exponent of 96 is just over decimal.MaxValue. This
// means that an exponent of 96 and above should overflow.
//
ulong mant;
// with SSE4.1 support ROUNDSD can be used
if (X86.Sse41.IsSupported)
mant = (ulong)(long)Math.Round(dbl);
else
if (input.Exponent >= 96)
Copy link
Contributor Author

@dakersnar dakersnar Jun 11, 2022

Choose a reason for hiding this comment

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

This should prevent all overflowing doubles from making it to Dragon4.

{
mant = (ulong)(long)dbl;
dbl -= (long)mant; // difference between input & integer
if (dbl > 0.5 || dbl == 0.5 && (mant & 1) != 0)
mant++;
Number.ThrowOverflowException(TypeCode.Decimal);
}

if (mant == 0)
return; // result should be zeroed out

if (power < 0)
uint flags = 0;
if (input < 0)
{
// Add -power factors of 10, -power <= (29 - 15) = 14.
//
power = -power;
if (power < 10)
{
uint pow10 = s_powers10[power];
ulong low64 = UInt32x32To64((uint)mant, pow10);
ulong hi64 = UInt32x32To64((uint)(mant >> 32), pow10);
result.Low = (uint)low64;
hi64 += low64 >> 32;
result.Mid = (uint)hi64;
hi64 >>= 32;
result.High = (uint)hi64;
}
else
{
// Have a big power of 10.
//
Debug.Assert(power <= 14);
UInt64x64To128(mant, s_ulongPowers10[power - 1], ref result);
}
flags = SignMask;
}
else
{
// Factor out powers of 10 to reduce the scale, if possible.
// The maximum number we could factor out would be 14. This
// comes from the fact we have a 15-digit number, and the
// MSD must be non-zero -- but the lower 14 digits could be
// zero. Note also the scale factor is never negative, so
// we can't scale by any more than the power we used to
// get the integer.
//
int lmax = power;
if (lmax > 14)
lmax = 14;

if ((byte)mant == 0 && lmax >= 8)
{
const uint den = 100000000;
ulong div = mant / den;
if ((uint)mant == (uint)(div * den))
{
mant = div;
power -= 8;
lmax -= 8;
}
}

if (((uint)mant & 0xF) == 0 && lmax >= 4)
{
const uint den = 10000;
ulong div = mant / den;
if ((uint)mant == (uint)(div * den))
{
mant = div;
power -= 4;
lmax -= 4;
}
}

if (((uint)mant & 3) == 0 && lmax >= 2)
{
const uint den = 100;
ulong div = mant / den;
if ((uint)mant == (uint)(div * den))
{
mant = div;
power -= 2;
lmax -= 2;
}
}

if (((uint)mant & 1) == 0 && lmax >= 1)
{
const uint den = 10;
ulong div = mant / den;
if ((uint)mant == (uint)(div * den))
{
mant = div;
power--;
}
}

flags |= (uint)power << ScaleShift;
result.Low64 = mant;
}
(result.Low, result.Mid, result.High, uint scale) = Number.Dragon4DoubleToDecimal(input, 29, true);

flags |= scale << ScaleShift;
result.uflags = flags;

return;
}

/// <summary>
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Decimal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ namespace System
// require a large number of significant digits and no round-off errors.
//
// The finite set of values of type Decimal are of the form m
// / 10e, where m is an integer such that
// -296 <; m <; 296, and e is an integer
// / 10^e, where m is an integer such that
// -2^96 <; m <; 2^96, and e is an integer
Copy link
Contributor Author

@dakersnar dakersnar Jun 11, 2022

Choose a reason for hiding this comment

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

Suggested change
// -2^96 <; m <; 2^96, and e is an integer
// -2^96 <= m <= 2^96, and e is an integer

Is there something I'm missing about the use of ; here?

Copy link
Member

@AntonLapounov AntonLapounov Jul 14, 2022

Choose a reason for hiding this comment

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

This comment got corrupted in June 2002. Originally it looked this way:

     * <p>The finite set of values of type <i>Decimal</i> are of the form <i>m</i>
     * / 10<sup><i>e</i></sup>, where <i>m</i> is an integer such that
     * -2<sup>96</sup> &lt; <i>m</i> &lt; 2<sup>96</sup>, and <i>e</i> is an integer
     * between 0 and 28 inclusive.

You should use <, not <=. Also please move / 10^e to the previous line.

// between 0 and 28 inclusive.
//
// Contrary to the float and double data types, decimal
Expand Down
Loading