Add a `copysign` function to f32 and f64 #55169

raphlinus · 2018-10-18T01:32:35Z

This patch adds a copysign function to the float primitive types. It is an exceptionally useful function for writing efficient numeric code, as it often avoids branches, is auto-vectorizable, and there are efficient intrinsics for most platforms.

I think this might work as-is, as the relevant copysign intrinsic is already used internally for the implementation of signum. It's possible that an implementation might be needed in japaric/libm for portability across all platforms, in which case I'll do that also.

Part of the work towards #55107

This patch adds a `copysign` function to the float primitive types. It is an exceptionally useful function for writing efficient numeric code, as it often avoids branches, is auto-vectorizable, and there are efficient intrinsics for most platforms. I think this might work as-is, as the relevant `copysign` intrinsic is already used internally for the implementation of `signum`. It's possible that an implementation might be needed in japaric/libm for portability across all platforms, in which case I'll do that also. Part of the work towards rust-lang#55107

rust-highfive · 2018-10-18T01:32:45Z

r? @joshtriplett

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

joshtriplett · 2018-10-18T03:58:44Z

The two versions have doc comments that explain the NAN behavior differently. Could you please make them match? I found the explanation on the f32 version much clearer; could you copy that to the f64 version?

I improved the f32 version and made a copy-paste error for f64.

raphlinus · 2018-10-18T04:13:53Z

Definitely, and good catch. Was just a copy-paste error.

Who is a good person to look into the question of whether this might break exotic targets because llvm might generate a call to a (possibly unimplemented) library function for the general copysign intrinsic. Maybe @japaric?

nikic · 2018-10-18T11:04:19Z

Instead of a.copysign(b) something like a.with_sign_of(b) might make it more explicit which of the signs is being applied to which of the operands. On the other hand, copysign is standard terminology.

raphlinus · 2018-10-18T13:52:11Z

@nikic Good point, and I'm open to that. In this case, I'm going to suggest sticking with standard terminology, because I think the target user is much more likely to be studying or adapting numerical algorithms from other languages, rather than than writing Rust-only numerical algorithms. Also I think when studying asm output (especially wasm), it reduces the translation, understanding that copysign in the source should translate into the copysign intrinsic.

But if people strongly prefer a more explicit name, I'll change it.

joshtriplett · 2018-10-18T14:53:21Z

I think keeping the standard name copysign makes sense. And you've kept the order of arguments: a.copysign(b) matches copysign(a, b).

You could make it copysign_of for clarity, but even that divergence from the standard function might do more harm than good. I'll leave that up to you.

However, that brings to mind another thought. To prevent someone from thinking that this might modify the value in place, please add #[must_use] to require using the return value.

Added a #[must_use] annotation on copysign, per review feedback.

raphlinus · 2018-10-18T15:37:12Z

@joshtriplett Done. I agree with the rationale but will point out it's not consistent with the rest of the module. I think we can chalk that up to must_use not being available when the base library was done.

joshtriplett · 2018-10-18T16:45:23Z

LGTM. If you would like to seek feedback to confirm that this will work on obscure architectures, please feel free to do so and r=me afterward. You could also go ahead, on the theory that we already use many other intrinsics and this one doesn't seem drastically less likely to work.

raphlinus · 2018-10-18T21:50:09Z

I'm comfortable going ahead and seeing what breaks. If we get a failure on a specific architecture because of a missing _copysign, it should be pretty clear what to do next to fix it.

joshtriplett · 2018-10-18T22:25:10Z

@bors r+

bors · 2018-10-18T22:25:12Z

📌 Commit f08db6b has been approved by joshtriplett

Add a `copysign` function to f32 and f64 This patch adds a `copysign` function to the float primitive types. It is an exceptionally useful function for writing efficient numeric code, as it often avoids branches, is auto-vectorizable, and there are efficient intrinsics for most platforms. I think this might work as-is, as the relevant `copysign` intrinsic is already used internally for the implementation of `signum`. It's possible that an implementation might be needed in japaric/libm for portability across all platforms, in which case I'll do that also. Part of the work towards rust-lang#55107

Rollup of 7 pull requests Successful merges: - #54300 (Updated RELEASES.md for 1.30.0) - #55013 ([NLL] Propagate bounds from generators) - #55071 (Fix ICE and report a human readable error) - #55144 (Cleanup resolve) - #55166 (Don't warn about parentheses on `match (return)`) - #55169 (Add a `copysign` function to f32 and f64) - #55178 (Stabilize slice::chunks_exact(), chunks_exact_mut(), rchunks(), rchunks_mut(), rchunks_exact(), rchunks_exact_mut())

LukasKalbertodt · 2018-10-24T22:06:05Z

src/libstd/f32.rs

+    /// another.
+    ///
+    /// Equal to `self` if the sign of `self` and `y` are the same, otherwise
+    /// equal to `-y`. If `self` is a `NAN`, then a `NAN` with the sign of `y`


"otherwise equal to -self", and not "-y", right? Or am I confused now?

Also: I'd rephrase the first sentence to "Returns a number composed of the magnitude of self and the sign of y". That's more explicit.

And yes, I know the PR is already merged, but I guess we should fix this in a new PR, if I'm right about this typo.

@LukasKalbertodt You're absolutely right. I'm happy to fix this in a followup PR. Good catch!

@LukasKalbertodt

Thanks to @LukasKalbertodt for catching this. Addresses a comment raised in rust-lang#55169 after it was merged.

@LukasKalbertodt

…lett Fix doc for new copysign functions Thanks to \@LukasKalbertodt for catching this. Addresses a comment raised in rust-lang#55169 after it was merged.

@LukasKalbertodt

…lett Fix doc for new copysign functions Thanks to @LukasKalbertodt for catching this. Addresses a comment raised in rust-lang#55169 after it was merged.

crlf0710 · 2018-12-01T09:22:50Z

Well, it seems this is actually the tracking issue for copysign feature gate? Or is a separate tracking issue needed?

raphlinus · 2018-12-01T15:00:36Z

@crlf0710 I was told on IRC that this issue could serve as the tracking issue, for such a small feature. But I am not expert on the ways of Rust stabilization.

crlf0710 · 2018-12-01T18:08:55Z

@raphlinus The tracking issue should be an issue, not a PR, i think... @Centril

Centril · 2018-12-01T18:27:20Z

@crlf0710 is correct; there should be an open issue for it; take a look at https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3AC-tracking-issue+label%3AT-libs for inspiration.

raphlinus · 2018-12-01T18:35:54Z

Ok, will do. One reason I've been holding back is that was also planning on adding "round to nearest even" function as part of the same motivating work, but now I see that could take a long time (honestly, the motivation I have for it has somewhat stalled out), so no reason that should block this.

crlf0710 · 2018-12-02T10:49:36Z

Well i've just finished seeking for solutions of "round with ties to positive infinity" when i come to this issue yesterday. :)

…Sapin Stablize {f32,f64}::copysign(). Stablization PR for rust-lang#55169/rust-lang#58046. Please check if i'm doing it correctly. Is 1.35.0 good to go?

rust-highfive assigned joshtriplett Oct 18, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2018

Fix inconsistent documentation

9a2e702

I improved the f32 version and made a copy-paste error for f64.

Add must_use on copysign

f08db6b

Added a #[must_use] annotation on copysign, per review feedback.

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2018

kennytm mentioned this pull request Oct 19, 2018

Rollup of 7 pull requests #55194

Merged

bors merged commit f08db6b into rust-lang:master Oct 19, 2018

SimonSapin mentioned this pull request Oct 22, 2018

ICE 'allocation missing in dead_alloc_map' in rustc_mir::interpret when compiling Servo #55223

Closed

LukasKalbertodt reviewed Oct 24, 2018

View reviewed changes

raphlinus deleted the copysign branch October 24, 2018 22:16

raphlinus mentioned this pull request Oct 24, 2018

Fix doc for new copysign functions #55328

Merged

kennytm pushed a commit to kennytm/rust that referenced this pull request Oct 25, 2018

Fix doc for new copysign functions

538f65e

Thanks to @LukasKalbertodt for catching this. Addresses a comment raised in rust-lang#55169 after it was merged.

SimonSapin mentioned this pull request Feb 1, 2019

Tracking issue for {f32, f64}::copysign #58046

Closed

crlf0710 mentioned this pull request Mar 28, 2019

Stablize {f32,f64}::copysign(). #59503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `copysign` function to f32 and f64 #55169

Add a `copysign` function to f32 and f64 #55169

raphlinus commented Oct 18, 2018

rust-highfive commented Oct 18, 2018

joshtriplett commented Oct 18, 2018

raphlinus commented Oct 18, 2018

nikic commented Oct 18, 2018

raphlinus commented Oct 18, 2018

joshtriplett commented Oct 18, 2018

raphlinus commented Oct 18, 2018

joshtriplett commented Oct 18, 2018

raphlinus commented Oct 18, 2018

joshtriplett commented Oct 18, 2018

bors commented Oct 18, 2018

LukasKalbertodt Oct 24, 2018

raphlinus Oct 24, 2018

crlf0710 commented Dec 1, 2018

raphlinus commented Dec 1, 2018

crlf0710 commented Dec 1, 2018

Centril commented Dec 1, 2018

raphlinus commented Dec 1, 2018

crlf0710 commented Dec 2, 2018

Add a copysign function to f32 and f64 #55169

Add a copysign function to f32 and f64 #55169

Conversation

raphlinus commented Oct 18, 2018

rust-highfive commented Oct 18, 2018

joshtriplett commented Oct 18, 2018

raphlinus commented Oct 18, 2018

nikic commented Oct 18, 2018

raphlinus commented Oct 18, 2018

joshtriplett commented Oct 18, 2018

raphlinus commented Oct 18, 2018

joshtriplett commented Oct 18, 2018

raphlinus commented Oct 18, 2018

joshtriplett commented Oct 18, 2018

bors commented Oct 18, 2018

LukasKalbertodt Oct 24, 2018

Choose a reason for hiding this comment

raphlinus Oct 24, 2018

Choose a reason for hiding this comment

crlf0710 commented Dec 1, 2018

raphlinus commented Dec 1, 2018

crlf0710 commented Dec 1, 2018

Centril commented Dec 1, 2018

raphlinus commented Dec 1, 2018

crlf0710 commented Dec 2, 2018

Add a `copysign` function to f32 and f64 #55169

Add a `copysign` function to f32 and f64 #55169