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 Tuple-based Math.DivRem overloads #45074

Merged
merged 8 commits into from
Dec 4, 2020
Merged

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Nov 22, 2020

This PR adds Math.DivRem APIs approved in #42156 (comment)

  1. Should we hide "legacy" APIs? (see Consider hiding legacy overloads for Math.DivRem #44829) I personally hate them for out int result being actually a remainder.
  2. Do we use Capitalized names for tuples as return types?
  3. I can try to optimize these APIs via single idiv since there is a good example by @CarolEidt Support mulx returning ValueTuple #37928 (furthermore, we can teach the JIT to recognize a sort of GT_DIVREM anywhere now) - do you think it's worth the effort? It was the main reason I decided to add these APIs actually 🙂)

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Nov 22, 2020

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

Issue Details

This PR adds Math.DivRem APIs approved in #42156 (comment)

  1. Should we hide "legacy" APIs? (see Consider hiding legacy overloads for Math.DivRem #44829) I personally hate them for out int result being actually a remainder.
  2. Do we use Capitalized names for tuples as return types?
  3. I can try to optimize these APIs via single idiv since there is a good example by @CarolEidt Support mulx returning ValueTuple #37928 (furthermore, we can teach the JIT to recognize a sort of GT_DIVREM anywhere) - do you think it's worth the effort? (it was the main reason I decided to add these APIs actually 🙂)
Author: EgorBo
Assignees: -
Labels:

area-System.Numerics, new-api-needs-documentation

Milestone: -

@tannergooding
Copy link
Member

Should we hide "legacy" APIs? (see #44829) I personally hate them for out int result being actually a remainder.

I think we are going to look at an analyzer. I don't know if hiding them is the correct approach, especially for users with muscle memory.

Do we use Capitalized names for tuples as return types?

Yes.

I can try to optimize these APIs via single idiv since there is a good example by @CarolEidt #37928 (furthermore, we can teach the JIT to recognize a sort of GT_DIVREM anywhere) - do you think it's worth the effort? (it was the main reason I decided to add these APIs actually 🙂)

I think its probably worth having long term. I'm sure there is a lot of code that doesn't use DivRem, either because they don't know it exists or because they don't think about it.

@jkotas
Copy link
Member

jkotas commented Nov 22, 2020

Do we use Capitalized names for tuples as return types?

Yes. This was discussed extensively in the past and this was the conclusion: #27939 (comment)

@davidfowl
Copy link
Member

Someday we'll have better generics 🙃

@EgorBo
Copy link
Member Author

EgorBo commented Nov 23, 2020

(u)short and (s)byte overloads are above the ALWAYS_INLINE limit and don't want to inline (even to a loop body):

weight= 10 : state   3 [ ldarg.0 ]
weight= 16 : state   4 [ ldarg.1 ]
weight= 35 : state  79 [ div ]
weight= 25 : state 155 [ conv.u2 ]
weight= 20 : state 199 [ stloc.0 -> ldloc.0 ]
weight= 10 : state   3 [ ldarg.0 ]
weight= 12 : state   7 [ ldloc.0 ]
weight= 16 : state   4 [ ldarg.1 ]
weight= -9 : state  78 [ mul ]
weight=-15 : state  77 [ sub ]
weight= 25 : state 155 [ conv.u2 ]
weight=227 : state 103 [ newobj ]
weight= 19 : state  42 [ ret ]

Inline candidate callsite is in a loop.  Multiplier increased to 3.
calleeNativeSizeEstimate=391
callsiteNativeSizeEstimate=115
benefit multiplier=3
threshold=345
Native estimate for function size exceeds threshold for inlining 39.1 > 34.5 (multiplier = 3)

I wonder if we should add AggressiveInlining for them, other overloads are fine.

@jkotas
Copy link
Member

jkotas commented Nov 23, 2020

Yes, I think it would be fine to add AggressiveInlining to all these methods.

EgorBo and others added 2 commits November 26, 2020 00:43
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit bba6c0b into dotnet:master Dec 4, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants