Skip to content

New lint [inefficient_pow] #11057

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

New lint [inefficient_pow] #11057

wants to merge 3 commits into from

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented Jul 1, 2023

changelog: New lint [inefficient_pow]

@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2023

r? @Alexendoo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 1, 2023
@Centri3
Copy link
Member Author

Centri3 commented Jul 1, 2023 •
edited
Loading

I'm not entirely sure how useful this lint is, considering the cost of using .pow() anyway isn't too large (see godbolt, there are a ton of branches though) but maybe it's desired somewhere ^^

@Centri3
Copy link
Member Author

Centri3 commented Jul 1, 2023 •
edited
Loading

Huh, I've just looked at the assembly for 1 << 200 (while ensuring rustc doesn't optimize it out) and it seems it uses shl edi,cl, with and ecx,31, which is, uh, weird. I guess it ands it with the size of the integer. That's probably why it breaks in a really weird way when it overflows 🤷‍♀️ At least without it, it wouldn't roll back over and instead be 1100 -> 1000.

Unfortunately shl doesn't set the overflow flag afaik so it can't really be a rustc issue. An issue might be desirable just in case somebody smarter than me knows a way to detect overflow on this (stackoverflow shows nothing particularly clever), that way rustc can optimize out .pow() and it'll still be correct even if it overflows, maybe you can shl edi,1 repeatedly but that's way slower and probably slower than just .pow().

@bors
Copy link
Contributor

bors commented Jul 1, 2023

☔ The latest upstream changes (presumably #11012) made this pull request unmergeable. Please resolve the merge conflicts.

@Centri3 Centri3 force-pushed the inefficientcl_power_of_two branch from b27a105 to be50786 Compare July 19, 2023 09:37
@Centri3 Centri3 force-pushed the inefficientcl_power_of_two branch from be50786 to a5bc149 Compare July 19, 2023 09:39
@bors
Copy link
Contributor

bors commented Jul 26, 2023

☔ The latest upstream changes (presumably #11115) made this pull request unmergeable. Please resolve the merge conflicts.

@Centri3
Copy link
Member Author

Centri3 commented Jul 30, 2023

I don't particularly like the suggested code, and think this should be an optimization pass in rustc instead. I'll work on that instead

@Centri3 Centri3 closed this Jul 30, 2023
@Centri3 Centri3 deleted the inefficientcl_power_of_two branch July 30, 2023 06:08
@Centri3 Centri3 mentioned this pull request Jul 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Alexendoo Alexendoo

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Centri3 @rustbot @bors @Alexendoo