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

Math/MathF.Truncate isn't an intrinsic and results in inefficient codegen #56931

Closed
Tracked by #47244
MichalPetryka opened this issue Aug 5, 2021 · 7 comments · Fixed by #65014
Closed
Tracked by #47244

Math/MathF.Truncate isn't an intrinsic and results in inefficient codegen #56931

MichalPetryka opened this issue Aug 5, 2021 · 7 comments · Fixed by #65014
Labels
Milestone

Comments

@MichalPetryka
Copy link
Contributor

Description

Math/MathF.Truncate results in bad codegen that calls into native modf which is pretty slow. Instead, it should be an intrinsic for vroundsd/vroundss like Ceiling and Floor are (and clang also does do it for truncate).

Configuration

Sharplab Core CLR 5.0.721.25508 on amd64

Regression?

No idea.

Data

Sharplab for Math
Sharplab for MathF
Godbolt clang for double
Godbolt clang for float

@MichalPetryka MichalPetryka added the tenet-performance Performance related issue label Aug 5, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Numerics untriaged New issue has not been triaged by the area owner labels Aug 5, 2021
@ghost
Copy link

ghost commented Aug 5, 2021

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

Issue Details

Description

Math/MathF.Truncate results in bad codegen that calls into native modf which is pretty slow. Instead, it should be an intrinsic for vroundsd/vroundss like Ceiling and Floor are (and clang also does do it for truncate).

Configuration

Sharplab Core CLR 5.0.721.25508 on amd64

Regression?

No idea.

Data

Sharplab for Math
Sharplab for MathF
Godbolt clang for double
Godbolt clang for float

Author: MichalPetryka
Assignees: -
Labels:

area-System.Numerics, tenet-performance, untriaged

Milestone: -

@MichalPetryka MichalPetryka changed the title Math/MathF.Truncate results isn't an intrinsic and results in inefficient codegen Math/MathF.Truncate isn't an intrinsic and results in inefficient codegen Aug 5, 2021
@tannergooding
Copy link
Member

This should be a relatively simple fix but is going to be for 7.0.0 at the earliest.

@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Aug 5, 2021
@tannergooding tannergooding added this to the 7.0.0 milestone Aug 5, 2021
@danmoseley
Copy link
Member

@MichalPetryka any interest in offering a PR?

@MichalPetryka
Copy link
Contributor Author

I've never worked with the JIT so I'd prefer somebody else to do it since I don't really know how sadly.

@danmoseley
Copy link
Member

Fair enough. Still, if you have an interest I expect @tannergooding could give pointers. 😄

@AraHaan
Copy link
Member

AraHaan commented Oct 28, 2021

Sure thing, here you go: char*, int*, uint*, void*, short*, ushort*, long*, ulong*, float*.

@tannergooding
Copy link
Member

This would be similar to the handling that exists for Ceiling, Floor, and Round.

Basically if you search for NI_System_Math_Ceiling, you'll find a number of entries and for the most part its just adding an additional entry to cover NI_System_Math_Truncate. You'd need to also ensure the managed side API is marked with [Intrinsic].

Basically the only places that aren't adding in a new nearly identical path for NI_System_Math_Truncate is in codegenarmarch.cpp and codegenxarch.cpp where you need to pick the right instruction and the right constant for the rounding mode, respectively.

It should be relatively straightforward overall and I'm happy to help walk you through it over DMs, the C# community discord (https://aka.ms/csharp-discord, specifically the #lowlevel channel), or more ad-hoc on the issue here.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 8, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants