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

LLVM is unable to eliminate bounds checks with widening multiply #65931

Open
dhardy opened this issue Oct 29, 2019 · 5 comments
Open

LLVM is unable to eliminate bounds checks with widening multiply #65931

dhardy opened this issue Oct 29, 2019 · 5 comments
Labels
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dhardy
Copy link
Contributor

dhardy commented Oct 29, 2019

Demonstration:

pub fn bar(rand: u32, arr: &[u8]) -> u8 {
    let mul = rand as u64 * arr.len() as u64;
    let hi = (mul >> 32) as u32;
    arr[hi as usize]
}

Note that hi must be less than arr.len(). LLVM is unable to prove this, and inserts a bounds check on array access. Extract from the compiled assembly:

	imulq	%rdx, %rsi
	shrq	$32, %rsi
	cmpq	%rdx, %rsi
	jae	.LBB0_2

Application: rust-random/rand#592

(Note: we have multiple implementations of widening multiply here.)

@nikic
Copy link
Contributor

nikic commented Oct 29, 2019

To clarify, the assumption for your particular example is a 32-bit target, right? With 32-bit usize that is.

@nikic
Copy link
Contributor

nikic commented Oct 29, 2019

Possible optimization: https://rise4fun.com/Alive/2Us This converts into an arr.len() != 0 check, as just optimizing the bounds check away entirely is illegal.

@dhardy
Copy link
Contributor Author

dhardy commented Oct 29, 2019

This is just a simplified example — if you follow my link, you'll see we have implementations for sizes up to and including u128 input as well as SIMD types. I believe the point applies to all of these, but haven't tested personally.

@Centril Centril 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 30, 2019
@nikic
Copy link
Contributor

nikic commented Nov 1, 2019

Partial fix up at https://reviews.llvm.org/D69686. This should handle the case where the array length is constant (which seems to be the actual motivating case).

@vks
Copy link
Contributor

vks commented Aug 24, 2021

If arr is changed to be a slice into an array of constant size, the bound checks are eliminated with Rust 1.54. So @nikic's partial fix seems to be working.

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. 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

4 participants