Skip to content

Use BigMul for 32x32=64 in decimal #93345

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

Merged
merged 6 commits into from
Nov 6, 2023
Merged

Use BigMul for 32x32=64 in decimal #93345

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3fa9604
Use BigMul for 32x32=64 in decimal
lilinus Oct 11, 2023
fab4430
remove UInt32x32To64
lilinus Oct 12, 2023
6f04db3
Revert "remove UInt32x32To64"
lilinus Oct 13, 2023
0572988
Add internal Math.BigMul(uint, uint)
lilinus Oct 13, 2023
495a8e5
Use x86 intrinsic in Math.BugMul(uint, uint) for 32 bit
lilinus Oct 13, 2023
8c3f5ca
Use unsigned Math.BigMul
lilinus Oct 17, 2023
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
15 changes: 1 addition & 14 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 @@ -184,20 +184,7 @@ private static ulong UInt32x32To64(uint a, uint b)

private static void UInt64x64To128(ulong a, ulong b, ref DecCalc result)
Copy link
Member

@tannergooding tannergooding Oct 11, 2023

Choose a reason for hiding this comment

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

This can be replaced, technically speaking, by Math.BigMul(ulong, ulong, out ulong) as well.

Then it can also be deleted.

Copy link
Contributor Author

@lilinus lilinus Oct 11, 2023

Choose a reason for hiding this comment

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

Yeah sure, NB that overflow check and insertion to DecCalc result are still in body below. Do we prefer to move those around to where this is now called or should i keep this method?

Copy link
Member

@tannergooding tannergooding Oct 11, 2023

Choose a reason for hiding this comment

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

Probably better to keep the extra checks centralized here and just defer the algorithm to Math.BigMul

{
ulong low = UInt32x32To64((uint)a, (uint)b); // lo partial prod
Copy link
Member

@tannergooding tannergooding Oct 11, 2023

Choose a reason for hiding this comment

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

Can you delete UInt32x32To64 as well?

Copy link
Contributor Author

@lilinus lilinus Oct 11, 2023

Choose a reason for hiding this comment

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

Could yes, but:

  • Unfortunately does not exist any BigMul method for two uint32.
  • It is used in a lot of places where narrowing/casting from ulongs to uints are done, and the code is pretty convoluted already. This shows intent quite nicely

Copy link
Contributor

@pentp pentp Nov 6, 2023

Choose a reason for hiding this comment

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

Did you measure the performance of this change? Last time I checked, a similar change caused a significant performance regression because the codegen for Bmi2.MultiplyNoFlags is suboptimal.

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(SR.Overflow_Decimal);
result.Low64 = low;
Expand Down