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

more efficient is_ascii methods #96141

Closed
wants to merge 4 commits into from
Closed

Conversation

gilescope
Copy link
Contributor

These formulations seem to compile down to less (optimised) machine code. (The 0b10_0000 is making it lower case like in to_digit here

digit = (self as u32 | 0b10_0000).wrapping_sub('a' as u32).saturating_add(10);
)

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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 17, 2022
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Apr 17, 2022
@rust-log-analyzer

This comment has been minimized.

@nikic
Copy link
Contributor

nikic commented Apr 17, 2022

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.

@rust-log-analyzer

This comment has been minimized.

@gilescope
Copy link
Contributor Author

gilescope commented Apr 17, 2022

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 llvm nowrap. (or do you mean that is_ascii_alphabetic is good but the other two don't optimise as well as they potentially could?)

@frewsxcv
Copy link
Member

Can you add comments in the code describing how this works?

@Mark-Simulacrum
Copy link
Member

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).

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2022
@nikic
Copy link
Contributor

nikic commented Apr 24, 2022

I need a bit more of a hint as to what's wrong as there's not much docs on llvm nowrap. (or do you mean that is_ascii_alphabetic is good but the other two don't optimise as well as they potentially could?)

Sorry, I meant to say that LLVM is supposed to optimize this and does implement a the necessary optimization, but the presence of nsw flags currently prevents it from triggering. It's something that needs to be fixed on the LLVM side, though of course we can directly use the optimized form in the meantime.

@nikic
Copy link
Contributor

nikic commented Apr 29, 2022

The issue on the LLVM side is fixed by llvm/llvm-project@982cbed.

@JohnCSimon
Copy link
Member

Ping from triage:
@gilescope what is the status of this PR? It hasn't been touched in a while.

@Dylan-DPC
Copy link
Member

Closing this pr as inactive

@Dylan-DPC Dylan-DPC closed this Jan 18, 2023
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants