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.Round[F](x, MidpointRounding) doesn't use VROUNDS[SD] outside of "to even" #98164

Closed
colejohnson66 opened this issue Feb 8, 2024 · 5 comments · Fixed by #98186
Closed
Labels
area-System.Numerics help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue

Comments

@colejohnson66
Copy link

colejohnson66 commented Feb 8, 2024

Description

It appears that Math[F].Round doesn't always use VROUNDS[SD] with an embedded round control. It does use VROUNDS[SD] for "to even" as that follows MXCSR

The "round control" bits of VROUNDS[SD] are as follows:

  • 00: nearest/to even
  • 01: down/to negative infinity
  • 10: up/to positive infinity
  • 11: truncate/to zero

Bit three of the immediate is set to use MXCSR.RC and cleared to use the embedded round control bits. It looks like the JIT only uses VROUND[SD] with "use MXCSR" set.

Data

Expected behavior occurs for "to even" rounding, but with the "use MXCSR" bit set - hence the suspicion. In this case, no harm, no foul.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKBuIGYACM58gdhoG8bHfmmAJhACuwADYxGAFRi4MACiGiJjAG7YxASkYBeAHw8+RgLLYMACwB0AJREA7AfPVi0jYwEsBABwju7GW2EHPwBzSykIAFFVGDtNAG4aAF8gA

using System;

public class Program
{
    public double Test(double val) =>
        Math.Round(val, MidpointRounding.ToEven);
}
Program.Test(Double)
    L0000: vzeroupper
    L0003: vroundsd xmm0, xmm0, xmm1, 4
    L0009: ret

However, using a mode that is not set in MXCSR calls into the framework for help instead of using an embedding rounding mode. For the below, VROUNDSD should be used with an immediate of b010 (ignore MXCSR, and use "round up/to infinity").

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKBuIGYACM58gdhoG8bHfmmAJhACuwADYxGAFRi4MACiGiJjAG7YxASkYBeAHw8+RgLLYMACwB0AJREA7AfPVi0jYwEsBABwju7GW2EHPwBzSykIAAUIXHcMd1UYAEk7ADM/OIBPTQBuGgBfIA===

using System;

public class Program
{
    public double Test(double val) =>
        Math.Round(val, MidpointRounding.ToPositiveInfinity);
}
Program.Test(Double)
    L0000: vzeroupper
    L0003: vmovaps xmm0, xmm1
    L0007: xor edx, edx
    L0009: mov r8d, 4
    L000f: mov rax, 0x7ff7e0257618
    L0019: jmp qword ptr [rax]

Expected behavior for this would be:

Program.Test(Double)
    L0000: vzeroupper
    L0003: vroundsd xmm0, xmm0, xmm1, 2
    L0009: ret
@colejohnson66 colejohnson66 added the tenet-performance Performance related issue label Feb 8, 2024
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 8, 2024
@ghost
Copy link

ghost commented Feb 8, 2024

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

Issue Details

Description

It appears that Math[F].Round doesn't always use VROUNDS[SD] with an embedded round control. It does use VROUNDS[SD] for "to even" as that follows MXCSR

The "round control" bits of VROUNDS[SD] are as follows:

  • 00: nearest/to even
  • 01: down/to negative infinity
  • 10: up/to positive infinity
  • 11: truncate/to zero

Bit three of the immediate is set to use MXCSR.RC and cleared to use the embedded round control bits. It looks like the JIT only uses VROUND[SD] with "use MXCSR" set.

Data

Expected behavior occurs for "to even" rounding, but with the "use MXCSR" bit set instead of "use embedded with 00", hence the suspicion. In this case, no harm, no foul.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKBuIGYACM58gdhoG8bHfmmAJhACuwADYxGAFRi4MACiGiJjAG7YxASkYBeAHw8+RgLLYMACwB0AJREA7AfPVi0jYwEsBABwju7GW2EHPwBzSykIAFFVGDtNAG4aAF8gA

using System;
public class Program
{
    public double Test(double val) =>
        Math.Round(val, MidpointRounding.ToEven);
}
Program.Test(Double)
    L0000: vzeroupper
    L0003: vroundsd xmm0, xmm0, xmm1, 4
    L0009: ret

However, using a mode that is not set in MXCSR calls into the framework for help instead of using an embedding rounding mode. For the below, VROUNDSD should be used with an immediate of b010 (ignore MXCSR, and use "round up/to infinity").

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKBuIGYACM58gdhoG8bHfmmAJhACuwADYxGAFRi4MACiGiJjAG7YxASkYBeAHw8+RgLLYMACwB0AJREA7AfPVi0jYwEsBABwju7GW2EHPwBzSykIAAUIXHcMd1UYAEk7ADM/OIBPTQBuGgBfIA===

using System;
public class Program
{
    public double Test(double val) =>
        Math.Round(val, MidpointRounding.ToPositiveInfinity);
}
Program.Test(Double)
    L0000: vzeroupper
    L0003: vmovaps xmm0, xmm1
    L0007: xor edx, edx
    L0009: mov r8d, 4
    L000f: mov rax, 0x7ff7e0257618
    L0019: jmp qword ptr [rax]
Author: colejohnson66
Assignees: -
Labels:

area-System.Numerics, tenet-performance, untriaged

Milestone: -

@tannergooding
Copy link
Member

It's typically preferred to just use the simpler dedicated API instead:

  • nearest/to even - Round
  • down/to negative infinity - Floor
  • up/to positive infinity - Ceiling
  • truncate/to zero - Truncate

We can of course optimize the explicit enum modes as well, but its not as high of a priority.

@colejohnson66
Copy link
Author

colejohnson66 commented Feb 8, 2024

I do see that the dedicated methods use the embedded round control. For example, Ceiling uses b1010 (inexact exception suppression, to positive infinity).

Would it be preferable to just have the enum variant forward to the other methods like it already does for "to even"? I personally prefer Round with an explicit direction, as "to positive infinity" is more clear than "ceiling" when talking about negative values.

@tannergooding
Copy link
Member

If you'd like to put up a PR, then feel free.

@tannergooding tannergooding added help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Feb 8, 2024
@MichalPetryka
Copy link
Contributor

MichalPetryka commented Feb 8, 2024

Would it be preferable to just have the enum variant forward to the other methods like it already does for "to even"? I personally prefer Round with an explicit direction, as "to positive infinity" is more clear than "ceiling" when talking about negative values.

The issue with that was that iirc those methods forward to the enum ones on platforms that aren't accelerated.
EDIT: I guess it'd be enough then to just make it inlineable then.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 8, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 9, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Numerics help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants