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

Use LLVM intrinsics for floating-point min/max #61408

Merged
merged 2 commits into from Jun 7, 2019

Conversation

Projects
None yet
8 participants
@varkor
Copy link
Member

commented May 31, 2019

Resurrection of #46926, now that the optimisation issues are fixed. I've confirmed locally that #61384 solves the issues.

I'm not sure if we're allowed to move the min/max methods from libcore to libstd: I can't quite tell what the status is from #50145. However, this is necessary to use the intrinsics.

Fixes #18384.

r? @SimonSapin
cc @rkruppe @nikic

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 31, 2019

⚠️ Warning ⚠️

  • These commits modify submodules.
@rkruppe

This comment has been minimized.

Copy link
Member

commented May 31, 2019

This is a breaking change, the libcore methods are available on stable no_std crates.

@varkor

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

It's bad that the breaking change doesn't break anything in our test suite. no_std should be tested more thoroughly.

What's the solution here, then? Do we need to resolve #50145 somehow? It'd be good to work out a path to solving it.

Show resolved Hide resolved src/libstd/f32.rs Outdated
@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

If we are confident that these LLVM intrinsics never emit calls to functions that are expected to be provided by libc (but rather, when the target doesn’t have a dedicated instruction, falls back to emitting a sequence of "normal" instructions inline), then it is fine to use them in libcore.

If not, then yes this is a case of #50145 and I don’t know how to start solving it.

Unfortunately I’ve had a hard time reading LLVM source code to be able to tell which intrinsics do what.

@nikic

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

@SimonSapin These intrinsics can produce calls to fminf, fmin, fmaxf and fmax, at the very least for targets without hard float support.

It seems like we already have https://github.com/rust-lang-nursery/libm included in compiler-builtins, though it lacks these particular functions and I don't know what the state is of using that for non-wasm targets.

@varkor

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

It seems like we already have rust-lang-nursery/libm included in compiler-builtins, though it lacks these particular functions

Implementing these four functions is straightforward, at least.

though it lacks these particular functions and I don't know what the state is of using that for non-wasm targets.

So, as I understand it, though LLVM supports providing implementations of these intrinsics on platforms without native support, it's unclear whether rustc uses rust-lang-nursery/libm on any platform but WASM.

However, this milestone seems to indicate that math functions are used in core, though we should confirm this. If this is correct, then if we define fminf, fmin, fmaxf and fmax in the Rust libm, we should be good to use these intrinsics in core.

@varkor

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

I've opened up a PR to add the min/max functions to libm: rust-lang-nursery/libm#180. Once this is merged, the version of libm that compiler-builtins uses can be updated. After that, the version of compiler-builtins that core uses can be updated. Then, we should be good to use the intrinsics in core, as they'll be lowered properly by LLVM.
(This should also mean other intrinsics can be used in core, as long as they exist in compiler-builtins.)

@varkor varkor force-pushed the varkor:fmin-fmax-llvm-intrinsics branch from 4356e02 to 0e5edc9 Jun 6, 2019

@varkor

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

@SimonSapin: this is now ready for re-review. This updates the version of compiler-builtins to one that includes the fmin(f)/fmax(f) libm function implementations, so LLVM should lower the intrinsics correctly on platforms that don't have native support. This means we should be able to use the LLVM intrinsics in libcore now.

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

@nikic

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

To verify that the fallback to fmin() in no_std environment really works, maybe add a test with +soft-float target feature? Though might require disabling some other things like SSE to work on x86_64.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

Although I filed #50145 it is only tracking the gap left by previous PRs and decisions. I don’t feel confident that I know the constraints and implications, so I’d prefer to defer this one.

r? @alexcrichton

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

@bors: r+

Thanks @varkor!

We've already got a good suite of tests for the intrinsic implementations in libm in the libm repository itself, and we're arguably missing integration tests here in terms of making sure that the LLVM lowering is satisfied by compiler-builtins, but we typically handle those sorts of bugs over time as it's really difficult to write tests for them specificall here in this repository without too much CI burden.

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

📌 Commit 0e5edc9 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

⌛️ Testing commit 0e5edc9 with merge c5295ac...

bors added a commit that referenced this pull request Jun 7, 2019

Auto merge of #61408 - varkor:fmin-fmax-llvm-intrinsics, r=alexcrichton
Use LLVM intrinsics for floating-point min/max

Resurrection of #46926, now that the optimisation issues are fixed. I've confirmed locally that #61384 solves the issues.

I'm not sure if we're allowed to move the `min`/`max` methods from libcore to libstd: I can't quite tell what the status is from #50145. However, this is necessary to use the intrinsics.

Fixes #18384.

r? @SimonSapin
cc @rkruppe @nikic
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing c5295ac to master...

@bors bors added the merged-by-bors label Jun 7, 2019

@bors bors merged commit 0e5edc9 into rust-lang:master Jun 7, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 7, 2019

📣 Toolstate changed by #61408!

Tested on commit c5295ac.
Direct link to PR: #61408

🎉 rls on windows: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jun 7, 2019

📣 Toolstate changed by rust-lang/rust#61408!
Tested on commit rust-lang/rust@c5295ac.
Direct link to PR: <rust-lang/rust#61408>

🎉 rls on windows: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.