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] const fn: count_zeros, count_ones, leading_zeros, trailing_zeros #51267

Closed
2 of 3 tasks
glandium opened this issue Jun 1, 2018 · 28 comments
Closed
2 of 3 tasks
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) C-feature-request Category: A feature request, i.e: not implemented / a PR.

Comments

@glandium
Copy link
Contributor

glandium commented Jun 1, 2018

One thing that I would like to do is to compile-time assert that the log2 of an alignment is less a given size. A simple way to do log2 of a power of 2 is to use trailing_zeros. But trailing_zeros is not const, so it's not possible to do a compile-time check.

The equivalent __builtins in clang are const (example: https://godbolt.org/g/RxWczn) so llvm should be able to handle that.

However, adding const to the functions in core::num requires adding const to the corresponding intrinsics, and that is not supported (extern items cannot be 'const')

@hanna-kruppe
Copy link
Contributor

The underlying intrinsics are const as of #51171, now we just need to make the stable wrapper functions const (possibly #[rustc_const_unstable]).

The equivalent __builtins in clang are const (example: https://godbolt.org/g/RxWczn) so llvm should be able to handle that.

LLVM is irrelevant. Clang is evaluating the built-in in the frontend (check the generated IR at -O0), and rustc is doing it in miri. Both have to do that since the value of a constant can be needed during type checking.

@glandium
Copy link
Contributor Author

glandium commented Jun 1, 2018

hah, I tried an hour too early!

@glandium
Copy link
Contributor Author

glandium commented Jun 1, 2018

There's a bootstrapping problem, though. stage0 doesn't have support for those const intrinsics, so it won't accept the const keyword even behind a #[cfg(not(stage0))]. So it's either macro pain or wait for 1.29.

@nagisa
Copy link
Member

nagisa commented Jun 1, 2018

One thing that I would like to do is to compile-time assert that the log2 of an alignment is less a given size. A simple way to do log2 of a power of 2 is to use trailing_zeros. But trailing_zeros is not const, so it's not possible to do a compile-time check.

Is const fn assert(x: usize) -> bool { x < (1 << LOG2) } not enough to do this check?

Not disagreeing, though, that it would be nice to have these operations be const fn :)

@oli-obk
Copy link
Contributor

oli-obk commented Jun 1, 2018

cc @faern

@oli-obk oli-obk added A-const-fn C-feature-request Category: A feature request, i.e: not implemented / a PR. A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) labels Jun 1, 2018
@faern
Copy link
Contributor

faern commented Jun 1, 2018

Yes. I created #51171 in order to then make swap_bytes, to_le, to_be, trailing_zeros, count_ones etc const. Was planning on doing this as soon as the stage0 compiler is bumped. I already have the code for it done :)

@faern
Copy link
Contributor

faern commented Jun 1, 2018

Initially I only planned on making swap_bytes and the endianess conversion const fns. But since we also merged making cttz, ctlz and ctpop constant intrinsics this could very well be extended to the integer methods using these intrinsics as well.

I made the appropriate methods on integers constant and pushed a feature branch to my fork, the diff can be viewed at https://github.com/rust-lang/rust/compare/master...faern:const-int-ops?expand=1. Please check if it makes all the methods you need const fns. To try that branch one must first point to a very new nightly rustc for stage0 in config.toml.

I'm going to try to get that feature branch merged into nightly as soon as Rust 1.27 comes out and the stage0 compiler is bumped. But yes, if I understand the way the stage0 compiler is bumped, this can't hit stable before Rust 1.29.

@glandium
Copy link
Contributor Author

glandium commented Jun 1, 2018

(unless #51171 is uplifted to beta)

@oli-obk
Copy link
Contributor

oli-obk commented Jun 1, 2018

unless #51171 is uplifted to beta

There is no motivation to do so. This is a feature, not a bugfix.

To try that branch one must first point to a very new nightly rustc for stage0 in config.toml.

Idk what the policy on that is, but maybe we can just bump the stage0 now?

@Mark-Simulacrum
Copy link
Member

stage0 must always be a beta compiler, but otherwise we can bump whenever. I'm not opposed to uplifting #51171 if it make some other patch possible, but that should ~never be the case. AFAICT, the correct approach here is to have the relevant methods (count_*) be made const fn with #[cfg(not(stage0)] and left as-is in stage0, which should permit landing this change whenever.

@glandium
Copy link
Contributor Author

glandium commented Jun 1, 2018

You can't do something like:

#[cfg(stage0)]
fn foo() {}
#[cfg(not(stage0))]
const fn foo() {}

The stage0 compiler still barfs about the const. So the only way to do this without a stage0 that has #51171 is a macro that can parse all kinds of function declarations.

@Mark-Simulacrum
Copy link
Member

I am somewhat surprised by that? Do you happen to have an error message?

@glandium
Copy link
Contributor Author

glandium commented Jun 1, 2018

Same as without a #[cfg] at all, that const externs are not supported or something along those lines.

@hanna-kruppe
Copy link
Contributor

I can't reproduce that (example), and it doesn't make sense that const fn restrictions would be checked before cfg stripping. Please double-check and show your code.

@Mark-Simulacrum
Copy link
Member

I wouldn't expect us to edit the intrinsic definitions but rather have the cfg's around the definitions in libcore: e.g.,

rust/src/libcore/num/mod.rs

Lines 284 to 288 in aa094a4

#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
pub fn count_zeros(self) -> u32 {
(!self).count_ones()
}
which aren't externs...

@glandium
Copy link
Contributor Author

glandium commented Jun 2, 2018

I can't reproduce that (example), and it doesn't make sense that const fn restrictions would be checked before cfg stripping. Please double-check and show your code.

https://play.rust-lang.org/?gist=5eab5ec94d7b452942b2c5b8ed15f6f7&version=nightly&mode=debug

But as @Mark-Simulacrum says, the intrinsic definitions actually don't need to change in the end.

@faern
Copy link
Contributor

faern commented Jun 2, 2018

Seems like I can make it work by duplicating all methods and marking the const version with #[cfg(not(stage0))] and vice versa. But it creates an awful lot of duplication. All the new const methods gets duplicated, including their documentation. More precisely 499 LOC extra compared to the change that only makes them const and has to wait for the next release.

I'm personally in no rush to get this merged now, as compared to around June 22 (1.27). Adding this conditional compilation and getting it in now would mean it could potentially hit stable in 1.28, right? So 500 LOC vs 6 weeks extra wait? :)

I can submit it as a PR now with the extra #[cfg(not(stage0))] if people think it's worth it 🤷‍♂️ But then also all higher level changes one wants to make to libstd/libcore with this cool feature also require duplication and conditional compilation with respect to stage0 I guess?

@nagisa
Copy link
Member

nagisa commented Jun 2, 2018

The lines of code of no concern as after release is cut the cfg(stage0) stuff is removed anyway. And you can definitely just have /// definitely docs for documentation for stage0.

@faern
Copy link
Contributor

faern commented Jun 2, 2018

I submitted PR #51299 to address this issue.

@scottmcm
Copy link
Member

scottmcm commented Jun 3, 2018

Should const wrapping_add, overflowing_mut, and friends be this issue or another?

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jun 4, 2018
const fn integer operations

A follow up to rust-lang#51171
Fixes rust-lang#51267

Makes a lot of the integer methods (`swap_bytes`, `count_ones` etc) `const fn`s. See rust-lang#51267 for a discussion about why this is wanted and the solution used.
@fbstj
Copy link
Contributor

fbstj commented Jun 4, 2018

is this not a tracking issue now?

@faern
Copy link
Contributor

faern commented Jun 5, 2018

This is now available on nightly under the const_int_ops feature gate. And yes, we likely need a tracking issue. I'm not sure how soon it's appropriate to publish a PR that marks this as stable?

@oli-obk oli-obk reopened this Jun 5, 2018
@oli-obk oli-obk changed the title count_zeros, count_ones, leading_zeros, trailing_zeros should be const [tracking issue] const fn: count_zeros, count_ones, leading_zeros, trailing_zeros Jun 5, 2018
@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 13, 2018

@scottmcm did you open an issue about wrapping_add and friends ? Those should be const too, and they should be behind this same feature gate (const_int_ops).

@scottmcm
Copy link
Member

@gnzlbg No, I didn't.

@faern
Copy link
Contributor

faern commented Jul 13, 2018

@glandium You can link to the stabilization PR #51364 in the initial post maybe? See you did that for the implementation PR.

@glandium
Copy link
Contributor Author

Done (note I didn't add the item lists to the initial comment, oli_obk did)

@mjbshaw
Copy link
Contributor

mjbshaw commented Dec 30, 2018

What's the status here? The top-level comment says the stabilization PR is done, but it was actually closed without being merged. const_int_ops is still unstable.

@Centril
Copy link
Contributor

Centril commented Jan 13, 2019

Done in #57234.

@Centril Centril closed this as completed Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) C-feature-request Category: A feature request, i.e: not implemented / a PR.
Projects
None yet
Development

No branches or pull requests