Skip to content

Tracking Issue for feature(nonzero_leading_trailing_zeros) #79143

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Tracked by #16
andjo403 opened this issue Nov 17, 2020 · 8 comments · Fixed by #84082
Closed
Tracked by #16

Tracking Issue for feature(nonzero_leading_trailing_zeros) #79143

andjo403 opened this issue Nov 17, 2020 · 8 comments · Fixed by #84082
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@andjo403
Copy link
Contributor

Tracking issue for trailing_zeros and leading_zeros for non zero types as introduced by #79114.
The feature gate for the issue is #![feature(nonzero_leading_trailing_zeros)].

On many architectures, this functions can perform better than trailing_zeros/leading_zeros on the underlying integer type, as special handling of zero can be avoided.

@andjo403 andjo403 added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Nov 17, 2020
@jonas-schievink jonas-schievink added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 17, 2020
@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Dec 16, 2020
@leonardo-m
Copy link

On many architectures, this functions can perform better than trailing_zeros/leading_zeros on the underlying integer type, as special handling of zero can be avoided.

It seems on my (common) architecture it doesn't give an advantage:

#![feature(nonzero_leading_trailing_zeros)]
use std::num::NonZeroU32;

pub fn foo1(x: u32) -> u32 {
    x.trailing_zeros()
}

pub fn foo2(x: NonZeroU32) -> u32 {
    x.trailing_zeros()
}

-O:

foo1:
    test    edi, edi
    je      .LBB0_2
    bsf     eax, edi
    ret
.LBB0_2:
    mov     eax, 32
    ret

foo2:
    bsf     eax, edi
    ret

-O -C target-cpu=native:

foo1:
    tzcnt   eax, edi
    ret

foo2:
    tzcnt   eax, edi
    ret

So the question is: is this NonZero method worth keeping? Are the architectures where it gives a performance advantage common enough?

@andjo403
Copy link
Contributor Author

but as you show in your example if you want to make a distributable binary and due to this not set the target-cpu there is a big difference

@leonardo-m
Copy link

The architecture I'm using is quite standard. A distributable binary could probably use it as "universal target".
I don't know much common is going to be an instruction like tzcnt in the future. If you think a similar instruction will be sufficiently uncommon then please ignore my comments here :-)

@m-ou-se
Copy link
Member

m-ou-se commented Mar 31, 2021

@rfcbot merge

cc @rust-lang/wg-const-eval

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 31, 2021

Team member @m-ou-se 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 31, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 31, 2021

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

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 31, 2021
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 10, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 10, 2021

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.

@andjo403
Copy link
Contributor Author

andjo403 commented Apr 11, 2021

started to make the stabilization PR but hit a problem I think that const_cttz needs to be stabilized for this to be stabilized

from https://github.com/rust-lang/rust/blob/master/library/core/src/intrinsics.rs#L16
//! If an intrinsic is supposed to be used from a const fn with a rustc_const_stable attribute,
//! the intrinsic's attribute must be rustc_const_stable, too. Such a change should not be done
//! without T-lang consulation, because it bakes a feature into the language that cannot be
//! replicated in user code without compiler support.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 12, 2021
…trailing_zeros, r=m-ou-se

Stabilize nonzero_leading_trailing_zeros

Stabilizing nonzero_leading_trailing_zeros and due to this also stabilizing the intrinsic cttz_nonzero

FCP finished here: rust-lang#79143 (comment)
`@rustbot` modify labels: +T-libs

Closes rust-lang#79143
@bors bors closed this as completed in 7ce470f Apr 13, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 15, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. 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.

7 participants