Add LLVM `minnum`/`maxnum` intrinsics by varkor · Pull Request #46926 · rust-lang/rust

varkor · 2017-12-21T21:03:15Z

LLVM’s intrinsics minnum and maxnum are now used for min and
max on f32 and f64. This resolves#18384.

LLVM’s intrinsics `minnum` and `maxnum` are now used for `min` and `max` on `f32` and `f64`. This resolvesrust-lang#18384.

rust-highfive · 2017-12-21T21:03:27Z

r? @joshtriplett

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

joshtriplett · 2017-12-21T21:33:12Z

LGTM, as soon as CI passes.

estebank · 2017-12-21T22:49:57Z

+/// Returns the minimum of two `f32` values.
+#[cfg(stage0)]
+pub unsafe fn minnumf32(x: f32, y: f32) -> f32 {
+ (if x < y || y != y { x } else { y }) * 1.0


Shouldn't you copy the deleted comments from libcore at least once here?

Good call. Even though these are only used for bootstrapping, they still ought to have that correctness warning.

Okay, I was wondering whether they were necessary if just used for bootstrapping. I'll add them back!

joshtriplett · 2017-12-21T22:52:36Z

+/// Returns the minimum of two `f32` values.
+#[cfg(stage0)]
+pub unsafe fn minnumf32(x: f32, y: f32) -> f32 {
+ (if x < y || y != y { x } else { y }) * 1.0


Good call. Even though these are only used for bootstrapping, they still ought to have that correctness warning.

estebank · 2017-12-21T23:05:07Z

@bors r=joshtriplett rollup

bors · 2017-12-21T23:05:08Z

📌 Commit a060814 has been approved by joshtriplett

Add LLVM `minnum`/`maxnum` intrinsics LLVM’s intrinsics `minnum` and `maxnum` are now used for `min` and `max` on `f32` and `f64`. This resolvesrust-lang#18384.

kennytm · 2017-12-23T14:05:42Z

@bors rollup- r-

These intrinsics (llvm_minnum_f64) are undefined on asm.js. See https://travis-ci.org/rust-lang/rust/jobs/320632051 for details.

varkor · 2017-12-23T23:07:41Z

It looks like in order to get these working with asm.js, I'll probably need to create a PR on the emscripten repository to implement them (I'm not sure how quick the turnaround is after a PR is merged on the repository — any idea what the process is after that @kennytm?). I'll try to do this soon.

EDIT: I've opened emscripten-core/emscripten#5978 and emscripten-core/emscripten-fastcomp#210 to address this issue. Let's see what happens there.

scottmcm · 2017-12-29T02:13:09Z

From #42423 (comment)

There is an LLVM intrinsic llvm.minnum.fNN. When compared with the pure Rust implementation, the generated assembly is generally worse on x86_64 and i686, and better on ARM and AArch64 (Both won't call fmaxf/fminf unless there is no hard-float support).

Is the x64 assembly better now?

hanna-kruppe · 2017-12-29T10:54:49Z

Running the test case from the linked comment through godbolt reveals that the x86 situation is still similar: identical asm for general min/max, but the intrinsic optimizes less well when used for clamping.

varkor · 2017-12-30T20:45:56Z

This is quite interesting — before submitting the PR, I checked that the intrinsics generated better instructions in isolation than the existing Rust implementation, but in @rkruppe's example, they're identical. Here's a very small change to the example — the only thing that's changed is the order of the conditions, but the generated asm is significantly worse.

if other > self || other.is_nan(){self}else{ other }// 16 instructions (min)if other.is_nan() || other > self{self}else{ other }// 11 instructions (min)

max is similar. Notice that it's the former, less efficient, version which is currently used in the standard library.

I hadn't spotted the issue shown by clamping, but given that the advantage the intrinsics demonstrate seems to be achievable by the Rust code, I imagine it'd be better to hold off from using the intrinsics for min and max for now.

What's the best way to proceed? Use Rust for min and max, but make the condition optimisation here? Is it worth having the intrinsics if they're not directly used in the standard library yet?

hanna-kruppe · 2017-12-30T20:56:12Z

Optimizing the Rust implementation would be great. I don't think there's any point in having the intrinsics if we're not going to use them. Very unfortunate that you've already spent the time on them, but at least another optimization came out of it :)

Swapping the conditions generates more efficient x86 assembly. See rust-lang#46926 (comment).

varkor · 2017-12-30T21:44:26Z

Let's close this PR for the time being, then. When LLVM gets a little bit better at optimising minnum/maxnum intrinsics we can look into this again.

Optimise min/max Swapping the conditions generates more efficient x86 assembly. See #46926 (comment). r? @rkruppe

@SimonSapin

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

Add LLVM minnum/maxnum intrinsics
de46292
LLVM’s intrinsics `minnum` and `maxnum` are now used for `min` and `max` on `f32` and `f64`. This resolvesrust-lang#18384.

rust-highfive assigned joshtriplett Dec 21, 2017

estebank reviewed Dec 21, 2017
View reviewed changes

joshtriplett requested changes Dec 21, 2017
View reviewed changes

Add comments to bootstrapping minnum/maxnum
a060814

kennytm added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 22, 2017

kennytm mentioned this pull request Dec 23, 2017
Rollup of 9 pull requests #46965
Closed

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 23, 2017

varkor mentioned this pull request Dec 23, 2017
llvm.minnum.f(32|64) and llvm.maxnum.f(32|64) intrinsics are unimplemented emscripten-core/emscripten#5976
Closed

kennytm added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 24, 2017

varkor added a commit to varkor/rust that referenced this pull request Dec 30, 2017
Optimise min/max
fba16d3
Swapping the conditions generates more efficient x86 assembly. See rust-lang#46926 (comment).

varkor mentioned this pull request Dec 30, 2017
Optimise min/max #47080
Merged

varkor closed this Dec 30, 2017

bors added a commit that referenced this pull request Dec 31, 2017
Auto merge of#47080- varkor:contrib-12, r=rkruppe
54d7285
Optimise min/max Swapping the conditions generates more efficient x86 assembly. See #46926 (comment). r? @rkruppe

varkor mentioned this pull request Jan 4, 2018
Use minnum/maxnum LLVM instrinsics for fmin/fmax #18384
Closed

varkor mentioned this pull request May 31, 2019
Use LLVM intrinsics for floating-point min/max #61408
Merged

Uh oh!

Uh oh!

Conversation

varkor commented Dec 21, 2017

Uh oh!

rust-highfive commented Dec 21, 2017

Uh oh!

joshtriplett commented Dec 21, 2017

Uh oh!

estebankDec 21, 2017

Choose a reason for hiding this comment

Uh oh!

joshtriplettDec 21, 2017

Choose a reason for hiding this comment

Uh oh!

varkorDec 21, 2017

Choose a reason for hiding this comment

Uh oh!

joshtriplettDec 21, 2017

Choose a reason for hiding this comment

Uh oh!

estebank commented Dec 21, 2017

Uh oh!

bors commented Dec 21, 2017

Uh oh!

kennytm commented Dec 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varkor commented Dec 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottmcm commented Dec 29, 2017

Uh oh!

hanna-kruppe commented Dec 29, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varkor commented Dec 30, 2017

Uh oh!

hanna-kruppe commented Dec 30, 2017

Uh oh!

varkor commented Dec 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kennytm commented Dec 23, 2017•
edited
Loading

varkor commented Dec 23, 2017•
edited
Loading

hanna-kruppe commented Dec 29, 2017•
edited
Loading