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

codegen regression for bool #101048

Closed
Nugine opened this issue Aug 26, 2022 · 8 comments · Fixed by #109895
Closed

codegen regression for bool #101048

Nugine opened this issue Aug 26, 2022 · 8 comments · Fixed by #109895
Assignees
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. 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. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@Nugine
Copy link
Contributor

Nugine commented Aug 26, 2022

Code

I tried this code:

https://rust.godbolt.org/z/3EW6zoffE

pub fn all_zero(data: &[u64]) -> bool {
    data.iter().copied().fold(true, |acc, x| acc & (x == 0))
}

I expected to see this happen:

.LBB0_3:
        cmp     qword ptr [rdi + rcx], 0
        sete    dl
        and     al, dl
        add     rcx, 8
        cmp     rsi, rcx
        jne     .LBB0_3

Instead, this happened:

.LBB0_3:
        test    al, al
        setne   dl
        cmp     qword ptr [rdi + rcx], 0
        sete    al
        and     al, dl
        add     rcx, 8
        cmp     rsi, rcx
        jne     .LBB0_3

There are two unnecessary instructions:

        test    al, al
        setne   dl

Version it worked on

It most recently worked on: Rust 1.63, Rust beta

Version with regression

rustc --version --verbose:

rustc 1.65.0-nightly (748038961 2022-08-25)
binary: rustc
commit-hash: 7480389611f9d04bd34adf41a2b3029be4eb815e
commit-date: 2022-08-25
host: x86_64-unknown-linux-gnu
release: 1.65.0-nightly
LLVM version: 15.0.0

@rustbot modify labels: +regression-from-stable-to-nightly -regression-untriaged

@Nugine Nugine added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Aug 26, 2022
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-untriaged Untriaged performance or correctness regression. labels Aug 26, 2022
@Nugine
Copy link
Contributor Author

Nugine commented Aug 27, 2022

@alion02
Copy link

alion02 commented Aug 27, 2022

The "canonical" code produces identical (and, since it has early exit, much more performant in most cases) assemblies between versions.

pub fn all_zero(data: &[u64]) -> bool {
    data.iter().all(|&x| x == 0)
}

@Nugine
Copy link
Contributor Author

Nugine commented Aug 28, 2022

The "canonical" code produces identical (and, since it has early exit, much more performant in most cases) assemblies between versions.

pub fn all_zero(data: &[u64]) -> bool {
    data.iter().all(|&x| x == 0)
}

But constant-time functions can not early exit.
The "fold bool" variant should also be optimized when compiled with nighly. So I think it's regression.

@nikic nikic added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-slow Issue: Problems and improvements with respect to performance of generated code. labels Aug 30, 2022
@nikic
Copy link
Contributor

nikic commented Aug 30, 2022

Upstream issue: llvm/llvm-project#57448

@apiraino apiraino added P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Aug 31, 2022
@pnkfelix
Copy link
Member

pnkfelix commented Sep 1, 2022

Aside: I think our labels here are a bit too general; while the text for "regression-from-stable-to-nightly" label itself says "Performance or correctness regression from stable to nightly", I think it should probably be revised to be restricted to "correctness or severe performance regression ..."

@rustbot label: -regression-from-stable-to-nightly

@rustbot rustbot removed the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Sep 1, 2022
@pnkfelix
Copy link
Member

pnkfelix commented Sep 1, 2022

I'm putting back the label for now; I want to get more consensus amongst the team about what the labels should be used for and what they denote (and how to get a narrower view when one wants to filter out cases like this)

@rustbot label: +regression-from-stable-to-nightly

@rustbot rustbot added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Sep 1, 2022
@wesleywiser wesleywiser added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Sep 22, 2022
@nikic nikic self-assigned this Sep 30, 2022
@nikic nikic added P-medium Medium priority and removed P-high High priority labels Oct 6, 2022
@nikic
Copy link
Contributor

nikic commented Oct 6, 2022

Fixed upstream and will be pulled in with the next LLVM major version upgrade. I'm downgrading the priority as there's no point in revisiting this in triage until that time.

@Mark-Simulacrum Mark-Simulacrum added this to the 1.65.0 milestone Oct 28, 2022
@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Dec 9, 2022
@nikic nikic added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Apr 3, 2023
@nikic
Copy link
Contributor

nikic commented Apr 3, 2023

Fixed by the LLVM 16 upgrade.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Apr 11, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 12, 2023
@bors bors closed this as completed in 73f40d4 Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. 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. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants