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

Make `abs`, `wrapping_abs`, `overflowing_abs` const functions #63786

Merged
merged 2 commits into from Sep 10, 2019

Conversation

@tspiteri
Copy link
Contributor

tspiteri commented Aug 21, 2019

This makes abs, wrapping_abs and overflowing_abs const functions like #58044 makes wrapping_neg and overflowing_neg const functions.

abs is made const by returning (self ^ -1) - -1 = !self + 1 = -self for negative numbers and (self ^ 0) - 0 = self for non-negative numbers. The subexpression self >> ($BITS - 1) evaluates to -1 for negative numbers and 0 otherwise. The subtraction overflows when self is min_value(), as we would be subtracting max_value() - -1; this is when abs should overflow.

wrapping_abs and overflowing_abs make use of wrapping_sub and overflowing_sub instead of the subtraction operator.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Aug 21, 2019

r? @cramertj

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

@Centril Centril added this to the 1.39 milestone Aug 21, 2019
@Centril Centril added the needs-fcp label Aug 21, 2019
@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Aug 22, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 22, 2019

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Aug 22, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Aug 30, 2019

Ping @withoutboats @Kimundi @sfackler in case y'all missed this. :)

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Aug 30, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Sep 9, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 10, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 10, 2019

📌 Commit adee559 has been approved by alexcrichton

Centril added a commit to Centril/rust that referenced this pull request Sep 10, 2019
Make `abs`, `wrapping_abs`, `overflowing_abs` const functions

This makes `abs`, `wrapping_abs` and `overflowing_abs` const functions like rust-lang#58044 makes `wrapping_neg` and `overflowing_neg` const functions.

`abs` is made const by returning `(self ^ -1) - -1` = `!self + 1` = `-self` for negative numbers and `(self ^ 0) - 0` = `self` for non-negative numbers. The subexpression `self >> ($BITS - 1)` evaluates to `-1` for negative numbers and `0` otherwise. The subtraction overflows when `self` is `min_value()`, as we would be subtracting `max_value() - -1`; this is when `abs` should overflow.

`wrapping_abs` and `overflowing_abs` make use of `wrapping_sub` and `overflowing_sub` instead of the subtraction operator.
bors added a commit that referenced this pull request Sep 10, 2019
Rollup of 8 pull requests

Successful merges:

 - #63786 (Make `abs`, `wrapping_abs`, `overflowing_abs` const functions)
 - #63989 (Add Yaah to clippy toolstain notification list)
 - #64256 (test/c-variadic: Fix patterns on powerpc64)
 - #64292 (lowering: extend temporary lifetimes around await)
 - #64311 (lldb: avoid mixing "Hit breakpoint" message with other output.)
 - #64330 (Clarify E0507 to note Fn/FnMut relationship to borrowing)
 - #64331 (Changed instant is earlier to instant is later)
 - #64344 (rustc_mir: buffer -Zdump-mir output instead of pestering the kernel constantly.)

Failed merges:

r? @ghost
@bors bors merged commit adee559 into rust-lang:master Sep 10, 2019
5 checks passed
5 checks passed
pr Build #20190907.22 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
self
}
pub const fn wrapping_abs(self) -> Self {
(self ^ (self >> ($BITS - 1))).wrapping_sub(self >> ($BITS - 1))

This comment has been minimized.

Copy link
@RalfJung

RalfJung Sep 10, 2019

Member

I am not a big fan of making code less readable just to please our const qualification. :/

Cc @oli-obk @eddyb

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 11, 2019

Member

We've had variables for a while, please use them.

This comment has been minimized.

Copy link
@nikic

nikic Sep 11, 2019

Contributor

Has it been verified that this optimizes to identical LLVM IR?

This comment has been minimized.

Copy link
@tspiteri

tspiteri Sep 11, 2019

Author Contributor

@eddyb Since this has already passed final comment period and been merged, shall I open a new PR to use variables?

This comment has been minimized.

Copy link
@tspiteri

tspiteri Sep 11, 2019

Author Contributor

@nikic I can't read LLVM IR; what I did do before submitting the PR was to use llvm-mca, and although the assembly is not identical, the performance measure from llvm-mca was the same, or if I remember well, in some tests the reverse throughput improved by something tiny like 0.1, but not regressed. I only used llvm-mca with i686 or x86-64 instructions, and I don't think I tested all types comprehensively.

This comment has been minimized.

Copy link
@tspiteri

tspiteri Sep 11, 2019

Author Contributor

@nikic Hmm, using variables as @eddyb suggested seems to hit two birds with one stone. If I insert a variable for the sign, I get one implementation and an alias: https://godbolt.org/z/m-seT0

Does that mean that the performance will not regress?

This comment has been minimized.

Copy link
@nikic

nikic Sep 11, 2019

Contributor

@tspiteri Oops, I simply made a typo in my test and that's why the result is different, duh :) It actually does produce the same IR, both with variable and without. So forget everything I said, this change is fine from the optimization perspective.

This comment has been minimized.

Copy link
@tspiteri

tspiteri Sep 11, 2019

Author Contributor

@nikic While abs and wrapping_abs seem to be fine performance-wise, overflowing_abs from this PR actually does suffer from the problem discussed, so I will not forget everything you said! :) It's easily fixed by making it return (self.wrapping_abs(), self == Self::min_value()).

This comment has been minimized.

Copy link
@RalfJung

RalfJung Sep 11, 2019

Member

And also, the code with the sign factored out is more readable. ;)

That said, I still think we should rather wait for CTFE to support conditionals than compromise code readability, and even the let sign version is still arcane magic. But that call is up to T-libs.

This comment has been minimized.

Copy link
@Lokathor

Lokathor Sep 19, 2019

Contributor

No, we should definitely make as many of these small ops into const as soon as we can.

@tspiteri tspiteri deleted the tspiteri:const-abs branch Sep 10, 2019
@Centril Centril added the const-hack label Sep 17, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 25, 2019
use `sign` variable in abs and wrapping_abs methods

This also makes the code easier to understand by hinting at the significance of `self >> ($BITS - 1)`.

Also, now `overflowing_abs` simply uses `wrapping_abs`, which is clearer and avoids a potential performance regression in the LLVM IR.

This PR follows from the discussion from rust-lang#63786.

r? @eddyb
cc @nikic
@TimDiekmann TimDiekmann referenced this pull request Nov 4, 2019
30 of 58 tasks complete
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 11, 2019
Pkgsrc changes:
 * Remove patch which no longer applies (but what about RPATH?)
 * Adapt a few patches to changed files upstream.

Upstream changes:

Version 1.39.0 (2019-11-07)
===========================

Language
--------
- [You can now create `async` functions and blocks with `async fn`,
  `async move {}`, and `async {}` respectively, and you can now call
  `.await` on async expressions.][63209]
- [You can now use certain attributes on function, closure, and function
  pointer parameters.][64010] These attributes include `cfg`, `cfg_attr`,
  `allow`, `warn`, `deny`, `forbid` as well as inert helper attributes used
  by procedural macro attributes applied to items. e.g.
  ```rust
  fn len(
      #[cfg(windows)] slice: &[u16],
      #[cfg(not(windows))] slice: &[u8],
  ) -> usize {
      slice.len()
  }
  ```
- [You can now take shared references to bind-by-move patterns in the
  `if` guards of `match` arms.][63118] e.g.
  ```rust
  fn main() {
      let array: Box<[u8; 4]> = Box::new([1, 2, 3, 4]);

      match array {
          nums
  //      ---- `nums` is bound by move.
              if nums.iter().sum::<u8>() == 10
  //                 ^------ `.iter()` implicitly takes a reference to `nums`.
          => {
              drop(nums);
  //          ----------- Legal as `nums` was bound by move and so we have ownership.
          }
          _ => unreachable!(),
      }
  }
  ```

Compiler
--------
- [Added tier 3\* support for the `i686-unknown-uefi` target.][64334]
- [Added tier 3 support for the `sparc64-unknown-openbsd` target.][63595]
- [rustc will now trim code snippets in diagnostics to fit in your terminal.]
  [63402] **Note** Cargo currently doesn't use this feature. Refer to
  [cargo#7315][cargo/7315] to track this feature's progress.
- [You can now pass `--show-output` argument to test binaries to print the
  output of successful tests.][62600]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
---------
- [`Vec::new` and `String::new` are now `const` functions.][64028]
- [`LinkedList::new` is now a `const` function.][63684]
- [`str::len`, `[T]::len` and `str::as_bytes` are now `const` functions.][63770]
- [The `abs`, `wrapping_abs`, and `overflowing_abs` numeric functions are
  now `const`.][63786]

Stabilized APIs
---------------
- [`Pin::into_inner`]
- [`Instant::checked_duration_since`]
- [`Instant::saturating_duration_since`]

Cargo
-----
- [You can now publish git dependencies if supplied with a `version`.]
  [cargo/7237]
- [The `--all` flag has been renamed to `--workspace`.][cargo/7241] Using
  `--all` is now deprecated.

Misc
----
- [You can now pass `-Clinker` to rustdoc to control the linker used
  for compiling doctests.][63834]

Compatibility Notes
-------------------
- [Code that was previously accepted by the old borrow checker, but rejected by
  the NLL borrow checker is now a hard error in Rust 2018.][63565] This was
  previously a warning, and will also become a hard error in the Rust 2015
  edition in the 1.40.0 release.
- [`rustdoc` now requires `rustc` to be installed and in the same directory to
  run tests.][63827] This should improve performance when running a large
  amount of doctests.
- [The `try!` macro will now issue a deprecation warning.][62672] It is
  recommended to use the `?` operator instead.
- [`asinh(-0.0)` now correctly returns `-0.0`.][63698] Previously this
  returned `0.0`.

[62600]: rust-lang/rust#62600
[62672]: rust-lang/rust#62672
[63118]: rust-lang/rust#63118
[63209]: rust-lang/rust#63209
[63402]: rust-lang/rust#63402
[63565]: rust-lang/rust#63565
[63595]: rust-lang/rust#63595
[63684]: rust-lang/rust#63684
[63698]: rust-lang/rust#63698
[63770]: rust-lang/rust#63770
[63786]: rust-lang/rust#63786
[63827]: rust-lang/rust#63827
[63834]: rust-lang/rust#63834
[63927]: rust-lang/rust#63927
[63933]: rust-lang/rust#63933
[63934]: rust-lang/rust#63934
[63938]: rust-lang/rust#63938
[63940]: rust-lang/rust#63940
[63941]: rust-lang/rust#63941
[63945]: rust-lang/rust#63945
[64010]: rust-lang/rust#64010
[64028]: rust-lang/rust#64028
[64334]: rust-lang/rust#64334
[cargo/7237]: rust-lang/cargo#7237
[cargo/7241]: rust-lang/cargo#7241
[cargo/7315]: rust-lang/cargo#7315
[`Pin::into_inner`]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.into_inner
[`Instant::checked_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.checked_duration_since
[`Instant::saturating_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.saturating_duration_since
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.