Skip to content

Suboptimal Assembly Output from a Modulo on a Power of Two #129795

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Altanis opened this issue Aug 30, 2024 · 9 comments
Closed

Suboptimal Assembly Output from a Modulo on a Power of Two #129795

Altanis opened this issue Aug 30, 2024 · 9 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Altanis
Copy link

Altanis commented Aug 30, 2024

I tried this code:

#[no_mangle]
pub fn index(key: usize, buckets: usize) -> usize {
    assert!(buckets.is_power_of_two());
    key % buckets
}

Since assert!(buckets.is_power_of_two()) is run (and .is_power_of_two() is an inbuilt function), the latter modulus operation should reduce to key & (buckets - 1).

Instead, the outputted assembly here shows that this reduction does not take place.

Meta

The compiler in the GodBolt link is version 1.80.0.

@Altanis Altanis added the C-bug Category: This is a bug. label Aug 30, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 30, 2024
@saethlin saethlin added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 31, 2024
@scottmcm
Copy link
Member

scottmcm commented Sep 5, 2024

This is llvm/llvm-project#58996, and needs to be fixed in LLVM. Rust itself won't do this optimization.

@scottmcm scottmcm added I-slow Issue: Problems and improvements with respect to performance of generated code. and removed C-bug Category: This is a bug. labels Sep 5, 2024
dtcxzyw added a commit to llvm/llvm-project that referenced this issue Sep 10, 2024
This patch tries to infer is-power-of-2 from assumptions. I don't see
that this kind of assumption exists in my dataset.
Related issue: rust-lang/rust#129795

Close #58996.
@dtcxzyw
Copy link

dtcxzyw commented Sep 10, 2024

@scottmcm @dianqk @nikic This issue has been fixed by llvm/llvm-project#107745. Please add llvm-fixed-upstream.

@dianqk
Copy link
Member

dianqk commented Sep 10, 2024

You can also add it. :)
@rustbot label +llvm-fixed-upstream

@rustbot rustbot added the llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes label Sep 10, 2024
@nikic
Copy link
Contributor

nikic commented Sep 10, 2024

@dtcxzyw Doesn't this issue need support for dominating condition rather than assumption?

@dtcxzyw
Copy link

dtcxzyw commented Sep 10, 2024

@dtcxzyw Doesn't this issue need support for dominating condition rather than assumption?

I am preparing a follow-up patch for this :)

@dianqk
Copy link
Member

dianqk commented Sep 10, 2024

@rustbot label -llvm-fixed-upstream

@rustbot rustbot removed the llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes label Sep 10, 2024
dtcxzyw added a commit to llvm/llvm-project that referenced this issue Sep 13, 2024
@veera-sivarajan
Copy link
Contributor

@rustbot label +llvm-fixed-upstream

@rustbot rustbot added the llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes label Nov 22, 2024
@nikic
Copy link
Contributor

nikic commented Feb 20, 2025

Fixed by LLVM 20 upgrade.

@nikic nikic added E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. and removed llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes labels Feb 20, 2025
jieyouxu added a commit to jieyouxu/rust that referenced this issue Mar 16, 2025
Add codegen test for rust-lang#129795

Adds test for rust-lang#129795.

Min LLVM version is 20 because the optimization only happens since LLVM 20.
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 16, 2025
Rollup of 16 pull requests

Successful merges:

 - rust-lang#133055 (Expand `CloneToUninit` documentation.)
 - rust-lang#137147 (Add exclude to config.toml)
 - rust-lang#137864 (Don't drop `Rvalue::WrapUnsafeBinder` during GVN)
 - rust-lang#137890 (doc: clarify that consume can be called after BufReader::peek)
 - rust-lang#137956 (Add RTN support to rustdoc)
 - rust-lang#137968 (Properly escape regexes in Python scripts)
 - rust-lang#138082 (Remove `#[cfg(not(test))]` gates in `core`)
 - rust-lang#138275 (expose `is_s390x_feature_detected!` from `std::arch`)
 - rust-lang#138303 (Fix Ptr inconsistency in {Rc,Arc})
 - rust-lang#138309 (Add missing doc for intrinsic (Fix PR135334))
 - rust-lang#138323 (Expand and organize `offset_of!` documentation.)
 - rust-lang#138329 (debug-assert that the size_hint is well-formed in `collect`)
 - rust-lang#138465 (linkchecker: bump html5ever)
 - rust-lang#138471 (Clean up some tests in tests/ui)
 - rust-lang#138472 (Add codegen test for rust-lang#129795)
 - rust-lang#138484 (Use lit span when suggesting suffix lit cast)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 16, 2025
Rollup merge of rust-lang#138472 - KonaeAkira:master, r=Mark-Simulacrum

Add codegen test for rust-lang#129795

Adds test for rust-lang#129795.

Min LLVM version is 20 because the optimization only happens since LLVM 20.
@scottmcm
Copy link
Member

With the test added in #138472 I think we're good here 🎉

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants