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

Add System.Math.DivRem for ulong and other integers #42156

Closed
Maykeye opened this issue Sep 12, 2020 · 12 comments
Closed

Add System.Math.DivRem for ulong and other integers #42156

Maykeye opened this issue Sep 12, 2020 · 12 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics
Milestone

Comments

@Maykeye
Copy link

Maykeye commented Sep 12, 2020

Background and Motivation

Math.DivRem currently accepts only int32 and int64. When you are working with uint64, as of now it's easier to calculate division and remainder "by hand".
Which is not only surprising and slightly inconvenient, but it also may lead to suboptimal code if you are not aware of #5213

So for convenience I suggest adding DivRem for uint64 and since, these integers are not special, add for all other integer types while we are at it: uint32, [u]int16, [s]byte

Proposed API

namespace System
{
    public static partial class Math
    {
+          public static ulong DivRem(ulong a, ulong b, out ulong result)
+          public static uint DivRem(uint a, uint b, out uint result)
+          public static ushort DivRem(ushort a, ushort b, out ushort result)
+          public static short DivRem(short a, short b, out short result)
+          public static byte DivRem(byte a, byte b, out byte result)
+          public static sbyte DivRem(sbyte a, sbyte  b, out sbyte result)
          public static int DivRem(int a, int b, out int result)
     }

Usage Examples

Personally I ran into it when tried to use the function while formatting bit address inside of really large address space:

            bytes = Math.DivRem(bitOffset, 8ul, out bits);
            Console.WriteLine($"{bytes}+({bits}b)");

However looking for duplicates, I noticed that ulongs are implemented already(just not exposed), so here is the usage example from the very this exact repo.

Risks

There might be confusion what is called due to implicit conversions(when a=b=short, result=int)

@Maykeye Maykeye added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 12, 2020
@EgorBo
Copy link
Member

EgorBo commented Sep 12, 2020

I wish we could rename out result into out remainder 😕 it's so misleading

@ghost
Copy link

ghost commented Sep 12, 2020

Tagging subscribers to this area: @tannergooding, @pgovind, @jeffhandley
See info in area-owners.md if you want to be subscribed.

@jeffschwMSFT jeffschwMSFT added the untriaged New issue has not been triaged by the area owner label Sep 12, 2020
@Wraith2
Copy link
Contributor

Wraith2 commented Sep 12, 2020

It'd be neater to use a ValueTuple return and have (ulong Count, ulong Remainder) Math.DivRem(ulong value) et al.

@tannergooding tannergooding added this to the Future milestone Sep 14, 2020
@tannergooding
Copy link
Member

I believe ValueTuple is also the pattern that will produce better codegen with the changes @CarolEidt has been working on to allow multiple returns from a single instruction.

@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Sep 14, 2020
@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 labels Oct 27, 2020
@tannergooding
Copy link
Member

Using value tuple

+          public static (byte Quotient, byte Remainder) DivRem(byte left, byte right);
+          public static (sbyte Quotient, sbyte Remainder) DivRem(sbyte left, sbyte right);
+          public static (short Quotient, short Remainder) DivRem(short left, short right);
+          public static (ushort Quotient, ushort Remainder) DivRem(ushort left, ushort right);
+          public static (int Quotient, int Remainder) DivRem(int left, int right);
+          public static (uint Quotient, uint Remainder) DivRem(uint left, uint right);
+          public static (long Quotient, long Remainder) DivRem(long left, long right);
+          public static (ulong Quotient, ulong Remainder) DivRem(ulong left, ulong right);
+          public static (nint Quotient, nint Remainder) DivRem(nint left, nint right);
+          public static (nuint Quotient, nuint Remainder) DivRem(nuint left, nuint right);

@terrajobst
Copy link
Member

terrajobst commented Nov 17, 2020

Video

  • Let's move to the value tuple pattern
  • Let's only addd the tuple pattern for the new overloads
  • But let's also add value pattern overloads for the existing types
namespace System
{
    public static partial class Math
    {
        public static (byte Quotient, byte Remainder) DivRem(byte left, byte right);
        public static (sbyte Quotient, sbyte Remainder) DivRem(sbyte left, sbyte right);
        public static (short Quotient, short Remainder) DivRem(short left, short right);
        public static (ushort Quotient, ushort Remainder) DivRem(ushort left, ushort right);
        public static (int Quotient, int Remainder) DivRem(int left, int right);
        public static (uint Quotient, uint Remainder) DivRem(uint left, uint right);
        public static (long Quotient, long Remainder) DivRem(long left, long right);
        public static (ulong Quotient, ulong Remainder) DivRem(ulong left, ulong right);
        public static (nint Quotient, nint Remainder) DivRem(nint left, nint right);
        public static (nuint Quotient, nuint Remainder) DivRem(nuint left, nuint right);
     }
}

@terrajobst terrajobst 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 Nov 17, 2020
@Wraith2
Copy link
Contributor

Wraith2 commented Nov 17, 2020

It may also be worth considering an analyzer that recommends moving from the out version to valuetuple version since it seems the new version will perform better.

@CarolEidt
Copy link
Contributor

@tannergooding - I've updated my PR #37928, but we'll need to determine the appropriate naming (I simplistically appended a '2' to distinguish from the variant that returns a scalar).

@echesakovMSFT suggested we add a flag in the HW intrinsics table so that we don't have to special-case the methods that return ValueTuple in the JIT.

It may also be worth considering an analyzer that recommends moving from the out version to valuetuple version since it seems the new version will perform better.

For the mulx case, I found that I was able to get identical code by having the out version call the ValueTuple version, since the address-taking goes away once it's inlined.

@Symbai
Copy link

Symbai commented Nov 22, 2020

The legacy APIs have the benefit that it can be used in if-cases and saves one line:

if (Math.DivRem(12, 2, out int remainder) == 6)
{
  Console.WriteLine(remainder);
}

With tuple based it would not only be longer code but also more ugly:

var (result, remainder) = Math.DivRem(12, 2);
if (result  == 6)
{
  Console.WriteLine(remainder);
}

@tannergooding
Copy link
Member

@Symbai, actually you can do this using C# pattern matching:

if (Math.DivRem(12, 2) is { Quotient: 6, Remainder: var remainder })
{
    Console.WriteLine(remainder);
}

https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxNMAfAAgJgIwCwAUFgMwAEu5AwuQN7HlOUVYAs5AsgBQCU9jZkICWAM3LcAIsIBuAJThhuAViTlS/YVHrkAigFcA9jGFwAdqnIA2NQsjCzAE0QhyMiAnIJFEB888AvryCQkwMRKGRlHgAnNze9k6IvADcIaEB6ZkRzOlYeFYSDjB6RibmMGrF5Ha+SQj80vKK3NUANnCileTVCMIA5gAWMME5YelCWADsEh1d5AD0XgPDanMlAKTLQyNpY+TZAUA==

@Symbai
Copy link

Symbai commented Nov 23, 2020

Didn't know that 👀

@tannergooding
Copy link
Member

Closing, this was resolved in #45074

@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics
Projects
None yet
Development

No branches or pull requests

9 participants