Skip to content

Conversation

@lilinus
Copy link
Contributor

@lilinus lilinus commented Mar 2, 2024

Tried to make the code easier to read with all the DivRem stuff going on. It will also be much easier to leverage intrinsics in the helper methods (such as X86Base.DivRem) when they can be used properly.

I don't know how to compare benchmarks thought, to see there are no regressions in performance.

@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 2, 2024
@danmoseley
Copy link
Member

I don't know how to compare benchmarks thought, to see there are no regressions in performance.

Have you looked at the docs in dotnet/performance? They explain how to test private changes like this. If there's not coverage, you can add a test there.

@ghost
Copy link

ghost commented Mar 4, 2024

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

Issue Details

Tried to make the code easier to read with all the DivRem stuff going on. It will also be much easier to leverage intrinsics in the helper methods (such as X86Base.DivRem) when they can be used properly.

I don't know how to compare benchmarks thought, to see there are no regressions in performance.

Author: lilinus
Assignees: -
Labels:

area-System.Numerics, needs-area-label

Milestone: -

@tannergooding
Copy link
Member

Same general comments that I gave on #99212 apply here.

These really seems like some basic JIT optimizations we're missing and not something we should be adding hardcoded helpers around. Cases of ulong / uint and similar are areas that the JIT should be able to recognize and optimize itself, allowing this to light up anywhere its used and not require customized logic that only works for System.Decimal.

This would be particularly relevant for Arm64 which doesn't have specialized instructions allowing uint128 / uint64 or similar.

@lilinus
Copy link
Contributor Author

lilinus commented Mar 4, 2024

These really seems like some basic JIT optimizations we're missing and not something we should be adding hardcoded helpers around. Cases of ulong / uint and similar are areas that the JIT should be able to recognize and optimize itself, allowing this to light up anywhere its used and not require customized logic that only works for System.Decimal.

I agree with the missing JIT optimizations. The purpose of this PR was not to optimize, rather refactor a bit without impacting performance negatively.

Like you mentioned in #99212 this has to do with the DivRem operations. Thism PR just moves the workaround of calculating remainder/varries as difference from being spread out in the code to the helpers.

Main purpose of this PR is:

  • Make code more readable and easier to follow / maintain using the helpers. IMO it's just easier to follow what is going on with the divide operators with carries with these introduced helpers.
  • Homogenous DivRem methods already exists in Math, but these non-homogenous also (hopefully) conveys the inputs' and outputs' sizes of the operations better than before.
  • Operations and flow in existing methods stays the same, just using the new helper methods to (hopefully) improve maintainability.

@dotnet-policy-service
Copy link
Contributor

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@lilinus lilinus deleted the deccalc-divrem branch April 11, 2024 14:42
@github-actions github-actions bot locked and limited conversation to collaborators May 12, 2024
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.

4 participants