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

u128::count_ones is broken on aarch64-unknown-none-softfloat #80374

Closed
AaronKutch opened this issue Dec 26, 2020 · 9 comments
Closed

u128::count_ones is broken on aarch64-unknown-none-softfloat #80374

AaronKutch opened this issue Dec 26, 2020 · 9 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@AaronKutch
Copy link
Contributor

#![no_std]

pub fn u128_count_ones(x: u128) -> u32 {
    x.count_ones()
}

when compiled with cargo build --target aarch64-unknown-none-softfloat it gives:

error: could not compile `testlib`

Caused by:
  process didn't exit successfully: `rustc --crate-name testlib --edition=2018 src\lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debuginfo=2 -C metadata=ec790c6e5e4455bc -C extra-filename=-ec790c6e5e4455bc --out-dir C:\Users\aaron\Documents\github\testlib\target\aarch64-unknown-none-softfloat\debug\deps --target aarch64-unknown-none-softfloat -C incremental=C:\Users\aaron\Documents\github\testlib\target\aarch64-unknown-none-softfloat\debug\incremental -L dependency=C:\Users\aaron\Documents\github\testlib\target\aarch64-unknown-none-softfloat\debug\deps -L dependency=C:\Users\aaron\Documents\github\testlib\target\debug\deps` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)

This is preventing upgrading the compiler-builtins used by rustc. See rust-lang/compiler-builtins#398 and #79863.

@AaronKutch AaronKutch added the C-bug Category: This is a bug. label Dec 26, 2020
@LeSeulArtichaut LeSeulArtichaut added T-libs Relevant to the library team, which will review and decide on the PR/issue. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state labels Dec 26, 2020
@LeSeulArtichaut
Copy link
Contributor

From the backtrace in rust-lang/compiler-builtins#398 (comment), this appears to be a LLVM issue?

@LeSeulArtichaut LeSeulArtichaut added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 26, 2020
@nikic
Copy link
Contributor

nikic commented Dec 26, 2020

IR test case (still reproduces on LLVM master):

; RUN: llc -mtriple=aarch64 -mattr=-neon < %s
define i128 @test(i128 %i) {
  %c = call i128 @llvm.ctpop(i128 %i)
  ret i128 %c
}

declare i128 @llvm.ctpop(i128)

@nikic
Copy link
Contributor

nikic commented Dec 26, 2020

The problem is that https://github.com/llvm/llvm-project/blob/b21840751278128ef6942074ae89ea63c9c6ac58/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp#L15907-L15909 does not check whether the LowerCTPOP() call is successful and pushes an empty SDValue as the result.

@AaronKutch
Copy link
Contributor Author

I was able to remove the reference to u128::count_ones in compiler-builtins, so this isn't a severe issue. But, it should definitely be fixed quickly so that future versions of LLVM are fixed.

@nikic
Copy link
Contributor

nikic commented Dec 26, 2020

I've put up https://reviews.llvm.org/D93825.

@nikic
Copy link
Contributor

nikic commented Mar 14, 2021

This should be fixed on nightly, as a result of the LLVM 12 upgrade.

@est31
Copy link
Member

est31 commented May 25, 2021

Can this be closed? Should one revert rust-lang/compiler-builtins#399 to make sure the bug is fixed?

@AaronKutch
Copy link
Contributor Author

We don't have to revert anything, count_ones was actually unused in compiler-builtins. It didn't produce an unused warning because it was part of a trait

@nikic
Copy link
Contributor

nikic commented Aug 28, 2021

Closing this as the upstream issue is fixed and we have long since updated to LLVM 12.

@nikic nikic closed this as completed Aug 28, 2021
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. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants