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

PrimInt: add reverse_bits() method #202

Merged
merged 4 commits into from
Jun 16, 2021
Merged

Conversation

Xiretza
Copy link
Contributor

@Xiretza Xiretza commented Feb 12, 2021

No description provided.

@cuviper
Copy link
Member

cuviper commented Feb 15, 2021

Unfortunately, it's a breaking change to add a new trait method without a default implementation, because we haven't prevented downstream crates from implementing this trait, and they would fail without this new addition.

Your implementation also requires Rust 1.37.

@Xiretza
Copy link
Contributor Author

Xiretza commented Feb 21, 2021

I see, that's unforunate. Are there any plans for a new major version (or, since it's still at 0.x, any new version that includes breaking changes)?

@tspiteri
Copy link
Contributor

tspiteri commented Feb 24, 2021

It is possible to write a default implementation of reverse_bits which only depends on PrimInt methods. For example this works for me and it also gives me similar assembly on x86_64 to the new primitive reverse_bits methods (the only difference I could spot is in instruction order).

fn one_per_byte<P: PrimInt>() -> P {
    // i8, u8: return 0x01
    // i16, u16: return 0x0101 = (0x01 << 8) | 0x01
    // i32, u32: return 0x01010101 = (0x0101 << 16) | 0x0101
    // ...
    let mut ret = P::one();
    let mut shift = 8;
    let mut b = ret.count_zeros() >> 3;
    while b != 0 {
        ret = (ret << shift) | ret;
        shift <<= 1;
        b >>= 1;
    }
    ret
}
fn reverse_bits<P: PrimInt>(i: P) -> P {
    let rep_01: P = one_per_byte();
    let rep_03 = (rep_01 << 1) | rep_01;
    let rep_05 = (rep_01 << 2) | rep_01;
    let rep_0f = (rep_03 << 2) | rep_03;
    let rep_33 = (rep_03 << 4) | rep_03;
    let rep_55 = (rep_05 << 4) | rep_05;

    // code above only used to determine rep_0f, rep_33, rep_55;
    // optimizer should be able to do it in compile time
    let mut ret = i.swap_bytes();
    ret = ((ret & rep_0f) << 4) | ((ret >> 4) & rep_0f);
    ret = ((ret & rep_33) << 2) | ((ret >> 2) & rep_33);
    ret = ((ret & rep_55) << 1) | ((ret >> 1) & rep_55);
    ret
}

(For rustc versions supporting reverse_bits, a method should still be provided in primitive implementations using the standard method, to potentially avoid code duplication when dependencies use both trait method and inherent method.)

@Xiretza
Copy link
Contributor Author

Xiretza commented Feb 26, 2021

I added @tspiteri's fallback behind an autocfg gate, but I haven't been able to confirm whether it actually works as designed - I can't find any traces of the fallback method in the output binary (with stable rustc) even if I delete the method in the trait implementation entirely.

build.rs Outdated Show resolved Hide resolved
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, but I'd like to see more tests, especially to cover signed/unsigned and different sizes for the fallback implementation. (Probably just unit tests, not cluttering the doc tests.)

@Xiretza Xiretza force-pushed the reverse-bits branch 3 times, most recently from da619bc to a94bbeb Compare April 13, 2021 20:19
@Xiretza
Copy link
Contributor Author

Xiretza commented Apr 13, 2021

Sorry for the noise, I kept running into CI issues that I didn't have locally.

@cuviper
Copy link
Member

cuviper commented Jun 16, 2021

Thanks!

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 16, 2021

@bors bors bot merged commit 92298b2 into rust-num:master Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants