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

Optimize char_try_from_u32 #94112

Merged
merged 2 commits into from
Feb 20, 2022
Merged

Optimize char_try_from_u32 #94112

merged 2 commits into from
Feb 20, 2022

Conversation

digama0
Copy link
Contributor

@digama0 digama0 commented Feb 18, 2022

The optimization was proposed by @falk-hueffner in https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Micro-optimizing.20char.3A.3Afrom_u32/near/272146171, and I simplified it a bit and added an explanation of why the optimization is correct. The generated code is 2 instructions shorter and uses 2 registers instead of 4 on x86.

The optimization was proposed by @falk-hueffner in https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Micro-optimizing.20char.3A.3Afrom_u32/near/272146171,  and I simplified it a bit and added an explanation of why the optimization is correct.
@rust-highfive
Copy link
Collaborator

r? @scottmcm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 18, 2022
@rust-log-analyzer

This comment has been minimized.

@scottmcm
Copy link
Member

Hmm, looks like we have all the edges tested in

assert_eq!(char::try_from(0_u32), Ok('\0'));
assert_eq!(char::try_from(0x61_u32), Ok('a'));
assert_eq!(char::try_from(0xD7FF_u32), Ok('\u{D7FF}'));
assert!(char::try_from(0xD800_u32).is_err());
assert!(char::try_from(0xDFFF_u32).is_err());
assert_eq!(char::try_from(0xE000_u32), Ok('\u{E000}'));
assert_eq!(char::try_from(0x10FFFF_u32), Ok('\u{10FFFF}'));
assert!(char::try_from(0x110000_u32).is_err());
assert!(char::try_from(0xFFFF_FFFF_u32).is_err());
, so this seems good. (I know it's right, from the zulip thread, but since this didn't add any tests I wanted to check.)

@bors r+

(Though I still think it'd be good to teach LLVM about this pattern, so we never need to do it again by hand.)

@bors
Copy link
Contributor

bors commented Feb 18, 2022

📌 Commit 7c3ebec has been approved by scottmcm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2022
@hkratz
Copy link
Contributor

hkratz commented Feb 18, 2022

Testing all possible input values (just for fun):

use std::intrinsics::transmute;

#[inline]
pub const fn char_try_from_u32_old(i: u32) -> Result<char, ()> {
    if (i > char::MAX as u32) || (i >= 0xD800 && i <= 0xDFFF) {
        Err(())
    } else {
        // SAFETY: checked that it's a legal unicode value
        Ok(unsafe { transmute(i) })
    }
}

#[inline]
pub const fn char_try_from_u32_new(i: u32) -> Result<char, ()> {
    if (i ^ 0xD800).wrapping_sub(0x800) >= 0x110000 - 0x800 {
        Err(())
    } else {
        // SAFETY: checked that it's a legal unicode value
        Ok(unsafe { transmute(i) })
    }
}

#[cfg(test)]
mod tests {
    use crate::*;

    #[test]
    fn test_char_try_from_u32() {
        for i in 0..=u32::MAX {
            assert_eq!(char_try_from_u32_new(i), char_try_from_u32_old(i));
        }
    }
}

$ cargo test --release
Compiling chartest v0.1.0 (/Users/hans/dev/spikes/chartest)
Finished release [optimized] target(s) in 0.31s
Running unittests (target/release/deps/chartest-d411d651ea1d838e)

running 1 test
test tests::test_char_try_from_u32 ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 6.37s

@lqd
Copy link
Member

lqd commented Feb 18, 2022

Alive2 also approves of the transformation. There was little chance that a superoptimizer would yield an incorrect simplified expression in the first place, but it's still a nice confirmation.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2022
Optimize char_try_from_u32

The optimization was proposed by `@falk-hueffner` in https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Micro-optimizing.20char.3A.3Afrom_u32/near/272146171,  and I simplified it a bit and added an explanation of why the optimization is correct. The generated code is 2 instructions shorter and uses 2 registers instead of 4 on x86.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2022
Optimize char_try_from_u32

The optimization was proposed by ``@falk-hueffner`` in https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Micro-optimizing.20char.3A.3Afrom_u32/near/272146171,  and I simplified it a bit and added an explanation of why the optimization is correct. The generated code is 2 instructions shorter and uses 2 registers instead of 4 on x86.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2022
Optimize char_try_from_u32

The optimization was proposed by ```@falk-hueffner``` in https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Micro-optimizing.20char.3A.3Afrom_u32/near/272146171,  and I simplified it a bit and added an explanation of why the optimization is correct. The generated code is 2 instructions shorter and uses 2 registers instead of 4 on x86.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2022
Optimize char_try_from_u32

The optimization was proposed by ````@falk-hueffner```` in https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Micro-optimizing.20char.3A.3Afrom_u32/near/272146171,  and I simplified it a bit and added an explanation of why the optimization is correct. The generated code is 2 instructions shorter and uses 2 registers instead of 4 on x86.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2022
Optimize char_try_from_u32

The optimization was proposed by `````@falk-hueffner````` in https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Micro-optimizing.20char.3A.3Afrom_u32/near/272146171,  and I simplified it a bit and added an explanation of why the optimization is correct. The generated code is 2 instructions shorter and uses 2 registers instead of 4 on x86.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2022
Optimize char_try_from_u32

The optimization was proposed by ``````@falk-hueffner`````` in https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Micro-optimizing.20char.3A.3Afrom_u32/near/272146171,  and I simplified it a bit and added an explanation of why the optimization is correct. The generated code is 2 instructions shorter and uses 2 registers instead of 4 on x86.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2022
…askrgr

Rollup of 14 pull requests

Successful merges:

 - rust-lang#93580 (Stabilize pin_static_ref.)
 - rust-lang#93639 (Release notes for 1.59)
 - rust-lang#93686 (core: Implement ASCII trim functions on byte slices)
 - rust-lang#94002 (rustdoc: Avoid duplicating macros in sidebar)
 - rust-lang#94019 (removing architecture requirements for RustyHermit)
 - rust-lang#94023 (adapt static-nobundle test to use llvm-nm)
 - rust-lang#94091 (Fix rustdoc const computed value)
 - rust-lang#94093 (Fix pretty printing of enums without variants)
 - rust-lang#94097 (Add module-level docs for `rustc_middle::query`)
 - rust-lang#94112 (Optimize char_try_from_u32)
 - rust-lang#94113 (document rustc_middle::mir::Field)
 - rust-lang#94122 (Fix miniz_oxide types showing up in std docs)
 - rust-lang#94142 (rustc_typeck: adopt let else in more places)
 - rust-lang#94146 (Adopt let else in more places)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7cd857d into rust-lang:master Feb 20, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants