-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Optimize Math.Round for arm64 and AwayFromZero for x64 #64016
Conversation
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsI noticed that people quite often explicitly set double Test(double x) => Math.Round(x, MidpointRounding.AwayFromZero); Current codegen: vzeroupper
vmovaps xmm0, xmm1
xor edx, edx
mov r8d, 1
jmp System.Math:Round(double,int,int):double ;; not inlined, is not hw accelerated for AwayFromZero
; Total bytes of code: 20 New codegen: vzeroupper
vandpd xmm0, xmm1, xmmword ptr [reloc @RWD00]
vorpd xmm0, xmm0, xmmword ptr [reloc @RWD16]
vaddpd xmm0, xmm0, xmm1
vroundsd xmm0, xmm0, xmm0, 3
ret
RWD00 dq 8000000000000000h, 8000000000000000h
RWD16 dq 3FDFFFFFFFFFFFFFh, 3FDFFFFFFFFFFFFFh
; Total bytes of code: 30 On ARM64 it's a single instruction for both modes.
|
Can you share your data source? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Src looks good to me. Please ensure there are tests that will exercise the new code paths. Thanks.
Failed job is unrelated. @tannergooding are the tests I added sufficient? Also, it seems like |
@tannergooding ping 🙂 |
I noticed that people quite often explicitly set
AwayFromZero
rounding forMath.Round
(btw in C++round(double)
usesAwayFromZero
by default while we use "accountant-friendly"ToEven
) to remind you the difference:so I decided to optimize it.
Current codegen:
New codegen:
cc @tannergooding
PS: I can guard my change with
RuntimeHelpers.IsConstArgument
but I think it worth inlining as is.