-
Notifications
You must be signed in to change notification settings - Fork 13k
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
more efficient is_ascii methods #96141
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
This is a "logical or" related optimization failure: https://rust.godbolt.org/z/oKabW74zr The logical or does not get converted to a bitwise or due to the nowrap flags (I believe) and that prevents the conditions from being combined. |
This comment has been minimized.
This comment has been minimized.
Isn't that a bitwise OR? ( https://llvm.org/docs/LangRef.html#or-instruction ) %_3 = or i32 %_5, 32 I need a bit more of a hint as to what's wrong as there's not much docs on |
Can you add comments in the code describing how this works? |
I think comments are definitely necessary if we go with this approach, though I'd also like to see some llvm-mca or similar analysis suggesting this is actually a win (not just marginally shorter assembly). It's definitely less clear in terms of readability, which also IMO has the downside of making it harder to inline these methods if you're OR'ing a few of them together mentally. (The documentation contains similar ranges but is for me a little harder to immediately understand). |
Sorry, I meant to say that LLVM is supposed to optimize this and does implement a the necessary optimization, but the presence of |
The issue on the LLVM side is fixed by llvm/llvm-project@982cbed. |
Ping from triage: |
Closing this pr as inactive |
These formulations seem to compile down to less (optimised) machine code. (The 0b10_0000 is making it lower case like in to_digit here
rust/library/core/src/char/methods.rs
Line 354 in ac8b118
is_ascii_alphabetic
https://rust.godbolt.org/z/nMdjWz4aG
is_ascii_alphanumeric
https://rust.godbolt.org/z/dejn8P9q5
is_ascii_hexdigit
https://rust.godbolt.org/z/nYsdEPj4e