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

Optimization regression: array argument somehow produces null check? #139415

Open
alex opened this issue Apr 5, 2025 · 5 comments
Open

Optimization regression: array argument somehow produces null check? #139415

alex opened this issue Apr 5, 2025 · 5 comments
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-codegen Area: Code generation I-slow Issue: Problems and improvements with respect to performance of generated code. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alex
Copy link
Member

alex commented Apr 5, 2025

Minimal reproducer:

#[no_mangle]
pub fn f1(v: [&mut usize; 1]) {
    for x in v {
        *x += 1;
    }
}

Results in:

f1:
        test    rdi, rdi
        je      .LBB0_2
        inc     qword ptr [rdi]
.LBB0_2:
        ret

Rust's semantics don't allow v to ever be NULL! I don't know where the test rdi, rdi is coming from!

This regressed between 1.78 and 1.79.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 5, 2025
@saethlin
Copy link
Member

saethlin commented Apr 5, 2025

Rust's semantics don't allow v to ever be NULL! I don't know where the test rdi, rdi is coming from!

Well, if you look at the MIR for this you can see where the check is coming from: https://rust.godbolt.org/z/zYoT5dhW1. It's from the IntoIter::next call which returns an Option<&mut usize> so of course we need to check the discriminant.

But I think something is deeply wrong here. The LLVM IR for f1 says it takes an i64 argument and that's not right, it should be ptr noalias noundef align 8 dereferenceable(8). We've lost the niche information when lowering to LLVM IR, which is probably why the optimization pipeline fell over.

@saethlin saethlin added A-codegen Area: Code generation A-ABI Area: Concerning the application binary interface (ABI) 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. labels Apr 5, 2025
@saethlin
Copy link
Member

saethlin commented Apr 5, 2025

Very good find, how did you come across this?

@alex
Copy link
Member Author

alex commented Apr 5, 2025

I was investigating whether you can do vectorized reference count incrementing, so my starting point was https://rust.godbolt.org/z/P1zKbGf3b.

That itself was downstream of numpy/numpy#28651, and thinking about what perf could be for numpy's object dtype... In short, its @nelhage's fault!

@the8472
Copy link
Member

the8472 commented Apr 5, 2025

Note that this is an owning array iterator which moves the references into the IntoIter struct first before iterating.
If you use v.iter_mut() you get a slice iterator instead which does optimize.

@scottmcm
Copy link
Member

scottmcm commented Apr 5, 2025

This is similar to #123183 -- it's about this particular ABI not getting parameter attributes that it could.

Getting range(i64 1, 0) on the parameter would also fix this (Demo: https://llvm.godbolt.org/z/5qn8Ed4qz), even if it doesn't turn into ptr nonnull.


EDIT: oh, #124533 (comment) looks like another similar case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-codegen Area: Code generation I-slow Issue: Problems and improvements with respect to performance of generated code. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. 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

5 participants