Skip to content

Failed analysis of index dominance #80075

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

Open
leonardo-m opened this issue Dec 16, 2020 · 9 comments
Open

Failed analysis of index dominance #80075

leonardo-m opened this issue Dec 16, 2020 · 9 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-mir-opt Area: MIR optimizations C-enhancement Category: An issue proposing an enhancement or a PR with one. 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

@leonardo-m
Copy link

leonardo-m commented Dec 16, 2020

rustc 1.50.0-nightly (f76ecd0 2020-12-15) on this code:

pub fn foo(data: &[u8], i: usize, j: usize) -> u8 {
    if i < data.len() && j <= i {
        data[j]
    } else {
        0
    }
}

Can't remove the array bound test (compiling even with aggressive optimizations):

foo:
    push    rax
    cmp     rdx, rsi
    jae     .LBB0_4
    cmp     rcx, rdx
    ja      .LBB0_4
    cmp     rcx, rsi
    jae     .LBB0_3
    mov     al, byte ptr [rdi + rcx]
    pop     rcx
    ret
.LBB0_4:
    xor     eax, eax
    pop     rcx
    ret
.LBB0_3:
    lea     rdx, [rip + .L__unnamed_1]
    mov     rdi, rcx
    call    qword ptr [rip + core::panicking::panic_bounds_check@GOTPCREL]
    ud2

This shows that value range analysis could be useful.

Godbolt: https://rust.godbolt.org/z/jPoY6Mnx6

@leonardo-m leonardo-m added the C-bug Category: This is a bug. label Dec 16, 2020
@camelid camelid added A-mir-opt Area: MIR optimizations C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed C-bug Category: This is a bug. labels Dec 16, 2020
@camelid
Copy link
Member

camelid commented Dec 16, 2020

I relabeled this as an enhancement proposal since to my understanding this isn't a bug.

@jrmuizel
Copy link
Contributor

I filed corresponding gcc and llvm bugs:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98357
https://bugs.llvm.org/show_bug.cgi?id=48544

@jrmuizel
Copy link
Contributor

It looks like the new ConstraintElimination llvm pass should fix this.

@leonardo-m
Copy link
Author

It looks like the new ConstraintElimination llvm pass should fix this.

That went better than expected. LLVM devs often deliver.

@camelid camelid added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Jan 23, 2021
@jrmuizel
Copy link
Contributor

jrmuizel commented Mar 1, 2021

I confirmed that: rustc --emit=asm -C passes=constraint-elimination -O test.rs with a rust built with llvm12 branch gives:

	.section	__TEXT,__text,regular,pure_instructions
	.macosx_version_min 10, 7
	.globl	__ZN4test3foo17hbf89db3e2fd7b5fcE
	.p2align	4, 0x90
__ZN4test3foo17hbf89db3e2fd7b5fcE:
	.cfi_startproc
	xorl	%eax, %eax
	cmpq	%rdx, %rsi
	jbe	LBB0_3
	cmpq	%rdx, %rcx
	ja	LBB0_3
	pushq	%rbp
	.cfi_def_cfa_offset 16
	.cfi_offset %rbp, -16
	movq	%rsp, %rbp
	.cfi_def_cfa_register %rbp
	movb	(%rdi,%rcx), %al
	popq	%rbp
LBB0_3:
	retq
	.cfi_endproc

@leonardo-m
Copy link
Author

You could also test issue #80620 :-)

@leonardo-m
Copy link
Author

Is -C passes=constraint-elimination part of -C opt-level=3?

@jrmuizel
Copy link
Contributor

jrmuizel commented Mar 2, 2021

I would be surprised if it was, but it's probably worth adding.

@nikic nikic added the I-slow Issue: Problems and improvements with respect to performance of generated code. label Aug 28, 2021
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@veera-sivarajan
Copy link
Contributor

Fixed since rustc 1.73.0: https://rust.godbolt.org/z/r6rebEhWP

@rustbot label +e-needs-test

@rustbot rustbot added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Mar 14, 2025
# 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. A-mir-opt Area: MIR optimizations C-enhancement Category: An issue proposing an enhancement or a PR with one. 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

7 participants