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

Tracking Issue for more const int functions #53718

Closed
59 tasks done
TimDiekmann opened this issue Aug 26, 2018 · 35 comments · Fixed by #80962
Closed
59 tasks done

Tracking Issue for more const int functions #53718

TimDiekmann opened this issue Aug 26, 2018 · 35 comments · Fixed by #80962
Assignees
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@TimDiekmann
Copy link
Member

TimDiekmann commented Aug 26, 2018

(none exhaustive) list of libcore::num, which should be a const fn:

  1. Requires conditionals. Tracking Issue: Tracking issue for RFC 2342, "Allow if and match in constants" #49146 (implemented on nightly)
  2. Requires loops. Tracking Issue: Tracking issue for RFC 2344, "Allow loop in constant evaluation" #52000 (implemented on nightly)

This issue has been assigned to @9999years via this comment.

@Centril Centril added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 28, 2018
@TheDarkula
Copy link
Contributor

transmute() just got merged. You can start on your last block now :)

@TimDiekmann
Copy link
Member Author

@TheDarkula Thank you, I'll start implementing them as soon as #53699 is merged.

@TimDiekmann
Copy link
Member Author

Added rest of const_int_conversion to #53697

@TimDiekmann
Copy link
Member Author

I'm currently rebasing #53697 and testing. Then the first bunch should be ready to merge.

bors added a commit that referenced this issue Sep 3, 2018
Add more const int ops

r? @oli-obk

Tracking Issue: #53718

list of `const fn`s in this PR:

- `feature = const_int_rotate`
  - `rotate_left`
  - `rotate_right`
- `feature = const_int_wrapping`
  - `wrapping_add`
  - `wrapping_sub`
  - `wrapping_mul`
  - `wrapping_shl`
  - `wrapping_shr`
- `feature = const_int_overflowing`
  - `overflowing_add`
  - `overflowing_sub`
  - `overflowing_mul`
  - `overflowing_shl`
  - `overflowing_shr`
- `feature = const_int_sign`
  - `is_positive`
  - `is_negative`
- `feature = const_int_conversion`
  - `reverse_bits`
  - `to_le_bytes`
  - `to_ne_bytes`
  - `from_be_bytes`
  - `from_le_bytes`
  - `from_ne_bytes`
  - `reverse_bits`
@mjbshaw
Copy link
Contributor

mjbshaw commented Dec 31, 2018

Adding a comment here so I don't have to hunt for the following related issue: #51267 is the tracking issue for feature = const_int_ops, which tracks:

  • count_zeros -- stabilized
  • count_ones -- stabilized
  • leading_zeros -- stabilized
  • trailing_zeros -- stabilized

@Lokathor
Copy link
Contributor

wrapping_neg isn't on the list here it seems (though overflowing_neg is)

@ecstatic-morse
Copy link
Contributor

It's possible to make a great number of these const without conditional statements being legal in const fn by using indexing (e.g. [x, -x][(x < 0) as usize]). However, it's a bad idea because the technique results in much less efficient generated code. You can check this by viewing ASM in the previous link. I'm posting this to avoid duplication of effort in case someone else has the same idea.

@mark-i-m
Copy link
Member

mark-i-m commented Jun 8, 2019

In fact, we are already doing that to make Vec::new() a const fn 😛

pub const fn new_in(a: A) -> Self {
// !0 is usize::MAX. This branch should be stripped at compile time.
// FIXME(mark-i-m): use this line when `if`s are allowed in `const`
//let cap = if mem::size_of::<T>() == 0 { !0 } else { 0 };
// Unique::empty() doubles as "unallocated" and "zero-sized allocation"
RawVec {
ptr: Unique::empty(),
// FIXME(mark-i-m): use `cap` when ifs are allowed in const
cap: [0, !0][(mem::size_of::<T>() == 0) as usize],
a,
}
}

@ecstatic-morse
Copy link
Contributor

@mark-i-m That's probably where I first saw this technique!

My point was that making a similar modification would likely slow down these arithmetic operations considerably. The use in Vec::new can be eliminated via const-propagation post-monomorphization, but that optimization doesn't apply here.

bors added a commit that referenced this issue Jun 8, 2019
Make `i*::signum` a `const fn`.

Ticks a box in #53718.

This uses a well-known branchless implementation of `signum`: `(n > 0) as i32 - (n < 0) as i32`.

Here's a [playground](https://play.rust-lang.org/?version=nightly&mode=release&edition=2018&gist=747cf191c4974bf66c9d75e509ae6e6e) comparing the two techniques. On x86 in release mode, the branchless implementation is able to replace a `mov` and `cmov` with a `sar` and `add`, so this should be a bit faster as well.

~~This is marked as a draft since I think I'll need to add `#[rustc_const_unstable]` somewhere. Perhaps the reviewer can point me in the right direction.~~
@ecstatic-morse
Copy link
Contributor

@TimDiekmann. Now that #61635 has been merged, signum is an unstable const fn. Could you edit the initial post to reflect this?

@mjbshaw
Copy link
Contributor

mjbshaw commented Jul 6, 2019

Some of these could be made const today without conditionals. For unsigned:

  • overflowing_div
  • wrapping_div
  • wrapping_div_euclid
  • wrapping_rem
  • wrapping_rem_euclid
  • overflowing_div
  • overflowing_div_euclid
  • overflowing_rem
  • overflowing_rem_euclid
  • div_euclid
  • rem_euclid
  • is_power_of_two

For u8:

  • is_ascii

@mjbshaw
Copy link
Contributor

mjbshaw commented Jul 6, 2019

Also, wrapping_neg and overflowing_neg should be updated in the OP. Both were made const in #58044

@jonas-schievink
Copy link
Contributor

is_power_of_two and next_power_of_two shouldn't require conditionals. See https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2 (replacing && with &) and https://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2, respectively.

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Nov 4, 2019

@jonas-schievink is_power_of_two was recently constified. I've updated the original post.

Currently, next_power_of_two uses a non-zero count trailing zeros intrinsic, so it might be hard to constify without regressing performance. It would be a good first issue for someone to try this out and see though.

@ecstatic-morse
Copy link
Contributor

Actually, quite a few of these are now implemented. I've edited the post to reflect this. If anyone sees something out-of-date, let me know.

@mjbshaw
Copy link
Contributor

mjbshaw commented Nov 5, 2019

@ecstatic-morse The intrinsic can be const and implemented in miri. That's what's done for trailing_zeros, for example.

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Nov 5, 2019

You need to handle zero somehow without branching, and you need to do so without regressing performance for non-const code.

(Also, I meant leading zeros above, my bad).

@SimonSapin
Copy link
Contributor

Should NonZero*::new fall in this category?

9999years added a commit to 9999years/rust that referenced this issue Dec 19, 2019
With rust-lang#49146 merged, NonZero*::new can be const; see rust-lang#53718.
9999years added a commit to 9999years/rust that referenced this issue Dec 19, 2019
With rust-lang#49146 merged, {u8,i8,u16,...}::overflowing_div and
::overflowing_rem can be const; see rust-lang#53718.
9999years added a commit to 9999years/rust that referenced this issue Dec 19, 2019
With rust-lang#49146 merged, these can be const; see rust-lang#53718.
9999years added a commit to 9999years/rust that referenced this issue Dec 19, 2019
With rust-lang#49146 merged, these can be const; see rust-lang#53718.
9999years added a commit to 9999years/rust that referenced this issue Dec 19, 2019
With rust-lang#49146 merged, these can be const; see rust-lang#53718.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 5, 2020
… r=oli-obk

Make more arithmetic functions unstably const

This is a smaller version of rust-lang#66884 (thanks @9999years) that constifies many of the arithmetic functions on integer primitives from rust-lang#53718 that were blocked on rust-lang#49146.

This makes the following things unstably const.

- `feature = const_int_unchecked_arith`
  - `intrinsics::unchecked_add`
  - `intrinsics::unchecked_sub`
  - `intrinsics::unchecked_mul`
  - `intrinsics::unchecked_div`
  - `intrinsics::unchecked_rem`

- `feature = const_checked_int_methods`
  - `checked_add`
  - `checked_sub`
  - `checked_mul`
  - `checked_div` (Uses `intrinsics::unchecked_div` internally)
  - `checked_rem` (Uses `intrinsics::unchecked_rem` internally)
  - `checked_neg`
  - `checked_shl`
  - `checked_shr`
  - `checked_abs`

- `feature = const_saturating_int_methods`
  - `saturating_mul`
  - `saturating_neg`  (Uses `intrinsics::unchecked_sub` internally)
  - `saturating_abs` (Uses `intrinsics::unchecked_sub` internally)

- `feature = const_wrapping_int_methods`
  - `wrapping_div`
  - `wrapping_rem`

- `feature = const_overflowing_int_methods`
  - `overflowing_div`
  - `overflowing_rem`

- `feature = const_euclidean_int_methods`
  - `checked_div_euclid`
  - `checked_rem_euclid`
  - `wrapping_div_euclid`
  - `wrapping_rem_euclid`
  - `overflowing_div_euclid`
  - `overflowing_rem_euclid`

Exponentiation and operations on the `NonZero` types are left to a later PR.

r? @oli-obk
cc @rust-lang/wg-const-eval @rust-lang/libs
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 10, 2020
…olnay

Make `num::NonZeroX::new` an unstable `const fn`

cc rust-lang#53718

These require `#[feature(const_if_match)]`, meaning they must remain unstable for the time being.
@ecstatic-morse
Copy link
Contributor

@rustbot release-assignment

@shamatar
Copy link
Contributor

Are these changes alone sufficient to allow to use arithmetic ops like “+” in a constant context or does is require landing of const traits (for e.g. std::ops::Add)?

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 20, 2020
…-obk

Make integer exponentiation methods unstably const

cc rust-lang#53718

This makes the following inherent methods on integer primitives into unstable `const fn`:
- `pow`
- `checked_pow`
- `wrapping_pow`
- `overflowing_pow`
- `saturating_pow`
- `next_power_of_two`
- `checked_next_power_of_two`
- `wrapping_next_power_of_two`

Only two changes were made to the implementation of these methods. First, I had to switch from the `?` operator, which is not yet implemented in a const context, to a `try_opt` macro. Second, `next_power_of_two` was using `ops::Add::add` (see the first commit) to "get overflow checks", so I switched to `#[rustc_inherit_overflow_checks]`. I'm not quite sure why the attribute wasn't used in the first place.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 20, 2020
…-obk

Make integer exponentiation methods unstably const

cc rust-lang#53718

This makes the following inherent methods on integer primitives into unstable `const fn`:
- `pow`
- `checked_pow`
- `wrapping_pow`
- `overflowing_pow`
- `saturating_pow`
- `next_power_of_two`
- `checked_next_power_of_two`
- `wrapping_next_power_of_two`

Only two changes were made to the implementation of these methods. First, I had to switch from the `?` operator, which is not yet implemented in a const context, to a `try_opt` macro. Second, `next_power_of_two` was using `ops::Add::add` (see the first commit) to "get overflow checks", so I switched to `#[rustc_inherit_overflow_checks]`. I'm not quite sure why the attribute wasn't used in the first place.
@tspiteri
Copy link
Contributor

I think that the {to,from}_{be,le,ne}_bytes methods should be made const in stable. While it is true that currently the implementation depends on e.g. transmute, that is a hidden implementation detail, and they could already be implemented as const stably with e.g. b[3] << 24 | b[2] << 16 | b[1] << 8 | b[0]. Of course, I'm not suggesting that the implementation itself should be changed, just that it should not stop them from being marked const in stable.

@jhpratt
Copy link
Member

jhpratt commented May 3, 2020

Would a PR making .rem_euclid a const fn on stable be accepted? It could be trivially implemented as ((x % y) + y) % y. This reasoning has been used for other methods in the past.

@tspiteri
Copy link
Contributor

tspiteri commented May 3, 2020

@jhpratt

It doesn't give the same results for negative y. 3.rem_euclid(-2) == 1, but ((3 % (-2)) + (-2)) % (-2) == -1. This could be solved by using y.abs().

But it would overflow for example when x == i32::MAX - 1 && y == i32::MAX.

@myrrlyn
Copy link

myrrlyn commented May 7, 2020

Can we stabilize the const_int_conversion gate, or is there a standing issue preventing stabilization? As has been mentioned upthread, these functions have equivalent const-able expressions, but as they are still unstable, these must be copied into each crate that needs them. This copying is bug-prone, so stabilization would reduce duplication and the possibility of error production.

@tspiteri
Copy link
Contributor

tspiteri commented May 7, 2020

@myrrlyn It's already stabilized in beta.

@KodrAus KodrAus added B-unstable Blocker: Implemented in the nightly compiler and unstable. Libs-Tracked Libs issues that are tracked on the team's project board. labels Jul 29, 2020
@Razican
Copy link
Contributor

Razican commented Sep 10, 2020

Hello, as far as I can tell, both [1] and [2] are now stable, right? so there is no extra blocking issue to implement/stabilize those, right? Is there something that can be done to get them stabilized?

@tspiteri
Copy link
Contributor

@Razican Most of [1] were marked stable in #73858, so they will be stabilized in 1.47. The PR did not include division and remainder because they can panic, so maybe they need another footnote.

To stabilize the rest that can be stabilized (the pow functions I think), someone needs to write a PR, which will then need to be approved, pass through FCP, and get merged.

@jhpratt
Copy link
Member

jhpratt commented Jan 13, 2021

#80962 is up to stabilize all remaining methods mentioned at the top of this issue. The diagnostic on nightly is functionally the same as already-stable equivalents (5 % 0 vs 5.rem_euclid(0), for example).

@bors bors closed this as completed in 4940dd4 Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.