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

Array indexed by enum cast to usize no longer has bounds check eliminated #102303

Closed
jnlt3 opened this issue Sep 26, 2022 · 9 comments · Fixed by #103584
Closed

Array indexed by enum cast to usize no longer has bounds check eliminated #102303

jnlt3 opened this issue Sep 26, 2022 · 9 comments · Fixed by #103584
Assignees
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. 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.

Comments

@jnlt3
Copy link

jnlt3 commented Sep 26, 2022

Rust compiler fails to eliminate bounds check when array is indexed by an enum cast to usize
Compiler explorer: https://godbolt.org/z/4o5cqT1eK

Code

pub enum Enum {
    A, B, C
}

pub fn func(inbounds: Enum, array: &[i16; 3]) -> i16 {
    array[inbounds as usize]
}

I expected the compiler to remove the bounds checks like it did previously however it failed to do so.

Version it worked on

It most recently worked on: Rust 1.63.0

Version with regression

rustc 1.64.0 (a55dd71d5 2022-09-19)
binary: rustc
commit-hash: a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52
commit-date: 2022-09-19
host: x86_64-unknown-linux-gnu
release: 1.64.0
LLVM version: 14.0.6

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

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

@rustbot label +T-compiler +A-codegen +I-slow

@rustbot rustbot added A-codegen Area: Code generation 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 Sep 26, 2022
@clubby789
Copy link
Contributor

Regressed in #96862

@nikic
Copy link
Contributor

nikic commented Sep 26, 2022

cc @oli-obk It looks like the necessary support code has already landed in #98332, so this should be a simple fix now?

@oli-obk
Copy link
Contributor

oli-obk commented Sep 27, 2022

@Dajamante is working on it

@scottmcm scottmcm added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Sep 27, 2022
@scottmcm
Copy link
Member

scottmcm commented Sep 27, 2022

Note that this still works:

pub fn func(inbounds: &Enum, array: &[i16; 3]) -> i16 {
    array[*inbounds as usize]
}

So this is also the classic "LLVM doesn't have range metadata on parameters" problem.

(When there's a load, it gets !range metadata !{i8 0, i8 3}, which LLVM can then use to remove the comparison. But passed by value that metadata is unavailable.)

EDIT: Filed #102389 to discuss with lang whether that should compile. It didn't used to.

@MinusKelvin
Copy link

Should that work? Enum is not Copy so *inbounds is moving out of a shared reference. Or at least, that's what 1.63 says; 1.64 compiles it without any warnings. The 1.64 release notes say this about drop behavior:

  • The Drop behavior of C-like enums cast to ints has changed. These are already discouraged by a compiler warning.

But I don't see why a change in drop behavior should affect whether a value is moved or not, and of course, there is no warning.

@FCLC
Copy link

FCLC commented Sep 28, 2022

Example of the specific behavior with IR and using the load "workaround" https://godbolt.org/z/YK79ErY5r

@apiraino
Copy link
Contributor

apiraino commented Oct 4, 2022

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 4, 2022
@ouz-a
Copy link
Contributor

ouz-a commented Oct 21, 2022

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. 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.