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

Cleanup NativeAOT math helpers #100375

Merged
merged 8 commits into from
Apr 5, 2024
Merged

Cleanup NativeAOT math helpers #100375

merged 8 commits into from
Apr 5, 2024

Conversation

MichalPetryka
Copy link
Contributor

Extracted from #98858.

cc @jkotas I'd like the cleanup here to be merged here before moving the Checked multiply helpers around.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 27, 2024
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.

LGTM. Thank you!

@jkotas
Copy link
Member

jkotas commented Apr 1, 2024

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member

jkotas commented Apr 2, 2024

The outer loop failures look unrelated.

Comment on lines +53 to +54
// We need to compare with the very next double to two63. 0x402 is epsilon to get us there.
if (value is > -two63 - 0x402 and < two63)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
// We need to compare with the very next double to two63. 0x402 is epsilon to get us there.
if (value is > -two63 - 0x402 and < two63)
if (value is > Math.BitDecrement(-two63) and < two63)

Would this work here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a compile-time constant

Copy link
Member

Choose a reason for hiding this comment

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

Would this work here?

Nope. BitDecrement is not a JIT intrinsic, and it is too complicated to make the JIT inline in and evaluate it into a constant.

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHunGATCAFdgAGxgMAsgAoAlBy49FxAOyTsGABYA6AEIBLDABEYYWPhgA7DFPJaADDIDcC7gF8aroA===

@jkotas jkotas merged commit b76baa8 into dotnet:main Apr 5, 2024
90 checks passed
@jkotas
Copy link
Member

jkotas commented Apr 5, 2024

Thank you!

MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Apr 9, 2024
dotnet#100375 broke optimized Windows x86 runs.
MichalStrehovsky added a commit that referenced this pull request Apr 9, 2024
#100375 broke optimized Windows x86 runs.
@jkotas jkotas mentioned this pull request Apr 11, 2024
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
* Cleanup NativeAOT math helpers

* Restore checked cast helpers

* Rename variables

---------

Co-authored-by: Jan Kotas <[email protected]>
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants