Skip to content
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

[API Proposal]: Add missing BigMul overload for uint x uint -> ulong #98346

Closed
lilinus opened this issue Feb 13, 2024 · 10 comments · Fixed by #103455
Closed

[API Proposal]: Add missing BigMul overload for uint x uint -> ulong #98346

lilinus opened this issue Feb 13, 2024 · 10 comments · Fixed by #103455
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@lilinus
Copy link
Contributor

lilinus commented Feb 13, 2024

Background and motivation

Many of the integer types available for other operations are missing for Math.BigMul

API Proposal

namespace System;

public static class Math
{
    public static Int128 BigMul(long a, long b);
    public static ulong BigMul(uint a, uint b);
    public static UInt128 BigMul(ulong a, ulong b);
}

public readonly struct Int32
{
    public static long BigMul(int a, int b);
}

public readonly struct Int64
{
    public static Int128 BigMul(long a, long b);
}

public readonly struct UInt32
{
    public static ulong BigMul(uint a, uint b);
}

public readonly struct UInt64
{
    public static UInt128 BigMul(ulong a, ulong b);
}

API Usage

// Fancy the value
var x = uint.MaxValue;
var y = uint.MaxValue - 1;
var z = Math.BigMul(x, y);

Alternative Designs

Only adding it to Uint32 class instead of Math. (BigMul doesnt exist on other concrete types yet, but can be taken in other api suggestion if relevant)

namespace System;

public readonly struct UInt32
{
    public static ulong BigMul(uint a, uint b);
}

Risks

No response

@lilinus lilinus added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 13, 2024
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 13, 2024
@ghost
Copy link

ghost commented Feb 13, 2024

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Many of the integer types available for other operations are missing for Math.BigMul

API Proposal

namespace System;

public static class Math
{
    public short BigMul(sbytet a, sbyte b);
    public ushort BigMul(bytet a, byte b);
    public short BigMul(bytet a, short b);
    public uint BigMul(ushort a, ushort b);
    public ulong BigMul(uint a, uint b);
}

API Usage

// Fancy the value
var x = uint.MaxValue;
var y = uint.MaxValue - 1;
var z = Math.BigMul(x, y);

Alternative Designs

No response

Risks

No response

Author: lilinus
Assignees: -
Labels:

api-suggestion, area-System.Numerics

Milestone: -

@huoyaoyuan
Copy link
Member

In fact, there's no arithmetic operator defined for small integer types. They need to be upcasted to 32bit. The default operator already behaves as BigMul.

@lilinus
Copy link
Contributor Author

lilinus commented Feb 13, 2024

Ah i see, the main one IMO that is missing is uint x uint -> ulong, suince we already have int x int -> long

@colejohnson66
Copy link

What's wrong with just using (ulong)left * (ulong)right? IMO, long BigMul(int, int) shouldn't exist.

@tannergooding
Copy link
Member

What's wrong with just using (ulong)left * (ulong)right? IMO, long BigMul(int, int) shouldn't exist.

It's not as clear and not as easy to take advantage of hardware specific optimizations, particularly on 32-bit platforms.

Ah i see, the main one IMO that is missing is uint x uint -> ulong, suince we already have int x int -> long

We have this one internally, so if you update the proposal we can discuss exposing it.

Notably, we may want to discuss whether or not this should be int.BigMul and uint.BigMul instead, since generic math has effectively soft-obsoleted Math and new APIs were planned to be static methods on the concrete types instead.

@lilinus
Copy link
Contributor Author

lilinus commented Feb 14, 2024

Updated suggestion accordingly @tannergooding

@lilinus lilinus changed the title [API Proposal]: Add missing Math.BigMul overloads [API Proposal]: Add missing BigMul overload for uint x uint -> ulong Feb 14, 2024
@MichalPetryka
Copy link
Contributor

Should there be some new IBigMultiplicable<TSelf, TResult> for this?

@lilinus
Copy link
Contributor Author

lilinus commented Mar 14, 2024

Should there be some new IBigMultiplicable<TSelf, TResult> for this?

Sounds reasonable. Maybe IBigMultiplicable<TSelf, TOther, TResult> so it is more aligned with e.g. IMultiplyOperators<TSelf,TOther,TResult>

Best to make separate api suggestion for that?

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Mar 14, 2024
@tannergooding tannergooding added this to the Future milestone Mar 14, 2024
@tannergooding
Copy link
Member

tannergooding commented Mar 14, 2024

Exposing a different interface for big multiplication is difficult and not necessarily extensible.

It only works with very precise types (T and a type that is effectively twice the bitwidth of T). It really needs something more akin to associated types to make work in the ways people would actually want. Without such a feature, you basically end up not being able to write generic code that actually works with BigMul the way people would want.

You could define a general signature that returns upper/lower halves in a tuple instead, but that can have its own problems as well.

@bartonjs
Copy link
Member

bartonjs commented Jun 13, 2024

Video

  • The question was raised of why add these on the Math type, since we have previously said that the type is soft-deprecated.
    • The answer is "because they're overloading an existing member."
  • There was a brief question about the applicability to generic math, but since that requires (TSelf, TBigger) or (TSelf, TSmaller) that's too complicated, so this finite set is all we get.
  • We'll leave the ones on Math as (a, b) to match the current method, but the ones on each type will use the more globally-consistent (left, right) naming.
  • There's no real need to do this for (s)byte -> (~u)short, or (u)short -> (u)int.
namespace System;

public static partial class Math
{
    public static Int128 BigMul(long a, long b);
    public static ulong BigMul(uint a, uint b);
    public static UInt128 BigMul(ulong a, ulong b);
}

public readonly partial struct Int32
{
    public static long BigMul(int left, int right);
}

public readonly partial struct Int64
{
    public static Int128 BigMul(long left, long right);
}

public readonly partial struct UInt32
{
    public static ulong BigMul(uint left, uint right);
}

public readonly partial struct UInt64
{
    public static UInt128 BigMul(ulong left, ulong right);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants