Skip to content

Cleanup some math helpers in DecimalCalc #76118

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
huoyaoyuan wants to merge 2 commits into from
Closed

Cleanup some math helpers in DecimalCalc #76118

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
105acba
Use BigMul(ulong)
huoyaoyuan Sep 24, 2022
87b9021
Use float.Exponent and double.Exponent
huoyaoyuan Sep 24, 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
44 changes: 3 additions & 41 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
Expand Up @@ -151,50 +151,14 @@ private ulong Low64

#region Decimal Math Helpers

private static unsafe uint GetExponent(float f)
{
// Based on pulling out the exp from this single struct layout
// typedef struct {
// ULONG mant:23;
// ULONG exp:8;
// ULONG sign:1;
// } SNGSTRUCT;

return (byte)(*(uint*)&f >> 23);
}

private static unsafe uint GetExponent(double d)
{
// Based on pulling out the exp from this double struct layout
// typedef struct {
// DWORDLONG mant:52;
// DWORDLONG signexp:12;
// } DBLSTRUCT;

return (uint)(*(ulong*)&d >> 52) & 0x7FFu;
}

private static ulong UInt32x32To64(uint a, uint b)
{
return (ulong)a * (ulong)b;
}

private static void UInt64x64To128(ulong a, ulong b, ref DecCalc result)
{
ulong low = UInt32x32To64((uint)a, (uint)b); // lo partial prod
ulong mid = UInt32x32To64((uint)a, (uint)(b >> 32)); // mid 1 partial prod
ulong high = UInt32x32To64((uint)(a >> 32), (uint)(b >> 32));
high += mid >> 32;
low += mid <<= 32;
if (low < mid) // test for carry
high++;

mid = UInt32x32To64((uint)(a >> 32), (uint)b);
high += mid >> 32;
low += mid <<= 32;
if (low < mid) // test for carry
high++;

ulong high = Math.BigMul(a, b, out ulong low);
if (high > uint.MaxValue)
Number.ThrowOverflowException(TypeCode.Decimal);
result.Low64 = low;
Expand Down Expand Up @@ -1528,8 +1492,7 @@ internal static void VarDecFromR4(float input, out DecCalc result)
// 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.
//
const uint SNGBIAS = 126;
int exp = (int)(GetExponent(input) - SNGBIAS);
int exp = input.Exponent + 1;
Copy link
Copy Markdown
Member

@tannergooding tannergooding Sep 24, 2022

Choose a reason for hiding this comment

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

The change is correct in that it matches current behavior, but there isn't anything explaining why its "off by one".

That is Single.Exponent and Double.Exponent return the exact and correct exponent, while this is the adjusting it to be one higher than expected. I've not sat down to work through "why" the adjustment is needed for decimal's scale but this is the type of scenario where there should really be a comment explaining it.

Copy link
Copy Markdown
Member Author

@huoyaoyuan huoyaoyuan Sep 25, 2022

Choose a reason for hiding this comment

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

The logic looks slightly different for float. The original code lacks &mask, I'm not sure if the failure is related since VS debugging is somehow broken for me. But I don't expect the logic of float to be different from double.

Copy link
Copy Markdown
Contributor

@dakersnar dakersnar Sep 26, 2022

Choose a reason for hiding this comment

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

These helpers should be fixed once this PR gets merged. We tabled it for .NET 7 but now that we are starting to work on .NET 8 it should be revisited.

Copy link
Copy Markdown
Member Author

@huoyaoyuan huoyaoyuan Sep 27, 2022

Choose a reason for hiding this comment

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

Thanks. Then I think it's better to wait for that PR.

if (exp < -94)
return; // result should be zeroed out

Expand Down Expand Up @@ -1695,8 +1658,7 @@ internal static void VarDecFromR8(double input, out DecCalc result)
// 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.
//
const uint DBLBIAS = 1022;
int exp = (int)(GetExponent(input) - DBLBIAS);
int exp = input.Exponent + 1;
if (exp < -94)
return; // result should be zeroed out

Expand Down