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

Don't use f64 shims for f32 cmath functions on non 32-bit x86 MSVC #68033

Merged
merged 1 commit into from Jan 16, 2020

Conversation

@ollie27
Copy link
Contributor

ollie27 commented Jan 8, 2020

These shims are only needed on 32-bit x86. Additionally since https://reviews.llvm.org/rL268875 LLVM handles adding the shims itself for the intrinsics.

These shims are only needed on 32-bit x86. Additionally since https://reviews.llvm.org/rL268875 LLVM handles adding the shims itself for the intrinsics.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 8, 2020

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

@ollie27

This comment has been minimized.

Copy link
Contributor Author

ollie27 commented Jan 8, 2020

One hesitation I have is that on for example x86_64 MSVC some of these functions give slightly different results which look like they're actually less accurate. That makes this technically a breaking change but I don't know if we have strong stability or accuracy guarantees for these functions.

@dtolnay dtolnay changed the title Don't use f64 shims for f32 cmath functions on none 32-bit x86 MSVC Don't use f64 shims for f32 cmath functions on non 32-bit x86 MSVC Jan 16, 2020
Copy link
Member

dtolnay left a comment

Looks good to me. Thanks!

It looks like the existing workaround was added in #26601 before the LLVM fix to support Windows XP. At some point we should drop XP support and remove the remaining shims... https://internals.rust-lang.org/t/consider-dropping-support-for-windows-xp/8745

Regarding accuracy, the comment in cmath.rs acknowledges that the f64 promoted operations do not exactly match the f32 operations but were only "accurate enough for now".

FYI @alexcrichton

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Jan 16, 2020

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 16, 2020

📌 Commit 084217a has been approved by dtolnay

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jan 16, 2020
Don't use f64 shims for f32 cmath functions on non 32-bit x86 MSVC

These shims are only needed on 32-bit x86. Additionally since https://reviews.llvm.org/rL268875 LLVM handles adding the shims itself for the intrinsics.
bors added a commit that referenced this pull request Jan 16, 2020
Rollup of 4 pull requests

Successful merges:

 - #68033 (Don't use f64 shims for f32 cmath functions on non 32-bit x86 MSVC)
 - #68153 (resolve: Point at the private item definitions in privacy errors)
 - #68244 (Enable leak sanitizer test case)
 - #68255 (Remove unused auxiliary file that was replaced with rust_test_helpers)

Failed merges:

r? @ghost
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jan 16, 2020
Don't use f64 shims for f32 cmath functions on non 32-bit x86 MSVC

These shims are only needed on 32-bit x86. Additionally since https://reviews.llvm.org/rL268875 LLVM handles adding the shims itself for the intrinsics.
bors added a commit that referenced this pull request Jan 16, 2020
Rollup of 5 pull requests

Successful merges:

 - #68033 (Don't use f64 shims for f32 cmath functions on non 32-bit x86 MSVC)
 - #68244 (Enable leak sanitizer test case)
 - #68255 (Remove unused auxiliary file that was replaced with rust_test_helpers)
 - #68263 (rustdoc: HTML escape codeblocks which fail syntax highlighting)
 - #68274 (remove dead code)

Failed merges:

r? @ghost
@bors bors merged commit 084217a into rust-lang:master Jan 16, 2020
4 checks passed
4 checks passed
pr Build #20200108.84 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@ollie27

This comment has been minimized.

Copy link
Contributor Author

ollie27 commented Jan 16, 2020

It looks like the existing workaround was added in #26601 before the LLVM fix to support Windows XP.

Just to be clear, these shims were added to support the i686-pc-windows-msvc target which was also added in that PR. They were erroneously applied to all MSVC target including x86_64 as well which is what this PR fixes.

At some point we should drop XP support and remove the remaining shims...

The shims are still needed even in 32-bit x86 Windows 10 so we won't be able to remove them unfortunately.

@ollie27 ollie27 deleted the ollie27:win_f32 branch Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.