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

portable simd: -Zmir-opt-level=3 causes LLVM ERROR #98016

Closed
matthiaskrgr opened this issue Jun 12, 2022 · 10 comments · Fixed by #113603
Closed

portable simd: -Zmir-opt-level=3 causes LLVM ERROR #98016

matthiaskrgr opened this issue Jun 12, 2022 · 10 comments · Fixed by #113603
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-mir-opt Area: MIR optimizations A-simd Area: SIMD (Single Instruction Multiple Data) 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. glacier ICE tracked in rust-lang/glacier. I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. PG-portable-simd Project group: Portable SIMD (https://github.com/rust-lang/project-portable-simd) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@matthiaskrgr
Copy link
Member

I tried this code:
code originally from ./src/test/codegen/simd-wide-sum.rs

#![crate_type = "lib"]
#![feature(portable_simd)]

use std::simd::Simd;
const N: usize = 8;

#[no_mangle]
// CHECK-LABEL: @wider_reduce_into_iter
pub fn wider_reduce_into_iter(x: Simd<u8, N>) -> u16 {
    // CHECK: zext <8 x i8>
    // CHECK-SAME: to <8 x i16>
    // CHECK: call i16 @llvm.vector.reduce.add.v8i16(<8 x i16>
    x.to_array().into_iter().map(u16::from).sum()
}

Meta

rustc --version --verbose:

rustc 1.63.0-nightly (ec55c6130 2022-06-10)
binary: rustc
commit-hash: ec55c61305eaf385fc1b93ac9a78284b4d887fe5
commit-date: 2022-06-10
host: x86_64-unknown-linux-gnu
release: 1.63.0-nightly
LLVM version: 14.0.5

When compile with -Zmir-opt-level=3, there is a LLVM_ERROR which does not occur with -Zmir-opt-level=2
rustc ./src/test/codegen/simd-wide-sum.rs --edition=2021 -Zmir-opt-level=3 -Zvalidate-mir

Invalid bitcast
  %10 = bitcast <8 x i8> %9 to [8 x i8]
in function wider_reduce_into_iter
LLVM ERROR: Broken function found, compilation aborted!
@matthiaskrgr matthiaskrgr added I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. 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. A-simd Area: SIMD (Single Instruction Multiple Data) C-bug Category: This is a bug. A-mir-opt Area: MIR optimizations PG-portable-simd Project group: Portable SIMD (https://github.com/rust-lang/project-portable-simd) labels Jun 12, 2022
@matthiaskrgr matthiaskrgr changed the title simd: -Zmir-opt-level=3 causes LLVM ERROR portable simd: -Zmir-opt-level=3 causes LLVM ERROR Jun 12, 2022
@eggyal
Copy link
Contributor

eggyal commented Jun 12, 2022

Due to inlining of Simd::<u8, 8>::to_array:

al@eggy rust % rustc +nightly ./src/test/codegen/simd-wide-sum.rs --edition=2021 -Zmir-opt-level=3 -Zvalidate-mir -Zfuel=simd_wide_sum=46
warning: optimization-fuel-exhausted: Inline Instance { def: Item(WithOptConstParam { did: DefId(2:23390 ~ core[a873]::core_simd::vector::{impl#0}::to_array), const_param_did: None }), substs: [u8, Const { ty: usize, val: Value(Scalar(0x0000000000000008)) }] } into MirSource { instance: Item(WithOptConstParam { did: DefId(0:10 ~ simd_wide_sum[31d5]::wider_reduce_into_iter), const_param_did: None }), promoted: None }

warning: 1 warning emitted

al@eggy rust % rustc +nightly ./src/test/codegen/simd-wide-sum.rs --edition=2021 -Zmir-opt-level=3 -Zvalidate-mir -Zfuel=simd_wide_sum=47
warning: optimization-fuel-exhausted: Inline Instance { def: Item(WithOptConstParam { did: DefId(2:8403 ~ core[a873]::iter::traits::iterator::Iterator::map), const_param_did: None }), substs: [std::array::IntoIter<u8, 8_usize>, u16, fn(u8) -> u16 {<u16 as std::convert::From<u8>>::from}] } into MirSource { instance: Item(WithOptConstParam { did: DefId(0:10 ~ simd_wide_sum[31d5]::wider_reduce_into_iter), const_param_did: None }), promoted: None }

Invalid bitcast
  %6 = bitcast <8 x i8> %5 to [8 x i8]
in function wider_reduce_into_iter
LLVM ERROR: Broken function found, compilation aborted!

al@eggy rust % rustc +nightly --version --verbose
rustc 1.63.0-nightly (99930ac7f 2022-06-11)
binary: rustc
commit-hash: 99930ac7f8cbb5d9b319b2e2e92794fd6f24f556
commit-date: 2022-06-11
host: x86_64-apple-darwin
release: 1.63.0-nightly
LLVM version: 14.0.5

@matthiaskrgr
Copy link
Member Author

does the no_mangle attr have done kind of effect? can't check on mobile right now

@eggyal
Copy link
Contributor

eggyal commented Jun 12, 2022

I think it's more likely the Simd struct's repr(simd) being ignored when performing that field access. cc #27731

@eggyal
Copy link
Contributor

eggyal commented Jun 12, 2022

A slightly simplified example:

#![crate_type = "lib"]
#![feature(repr_simd)]

#[repr(simd)]
pub struct Simd([u8; 8]);

fn to_array_inner<T>(x: Simd) -> [u8; 8] {
    x.0
}

pub fn to_array_outer(x: Simd) -> [u8; 8] {
    to_array_inner::<()>(x)
}

It only arises when inlining a generic function (in this case to_array_inner) that bitcasts a repr(simd).

@bjorn3
Copy link
Member

bjorn3 commented Jun 12, 2022

The difference seems to be that x.0 bitcasts a pointer to a vector, while the inlined to_array_inner bitcasts a vector directly. I suspect that the extra move that happens when to_array_inner is inlined causes vector value to be stored behind a pointer. I can reproduce the crash using just

#![crate_type = "lib"]
#![feature(repr_simd)]

#[repr(simd)]
pub struct Simd([u8; 8]);

pub fn to_array_outer(x: Simd) -> [u8; 8] {
    let y = x;
    y.0
}

to force an extra move. As for why to_array_inner puts the vector behind a pointer, the default rust abi passes vector types by-ref to ensure that changing target features doesn't change the abi.

@bjorn3
Copy link
Member

bjorn3 commented Jun 12, 2022

With --emit llvm-ir -Cno-prepopulate-passes -Zverify-llvm-ir that crashes at

Invalid bitcast
  %1 = bitcast <8 x i8> %y to [8 x i8], !dbg !11
LLVM ERROR: Broken module found, compilation aborted!
Compiler returned: 101

and without -Zverify-llvm-ir it gives the following LLVM ir:

define i64 @_ZN7example14to_array_outer17h4eae74749e7c0538E(<8 x i8>* %x) unnamed_addr #0 !dbg !5 {
  %0 = alloca [8 x i8], align 1
  %y = load <8 x i8>, <8 x i8>* %x, align 8, !dbg !10
  %1 = bitcast <8 x i8> %y to [8 x i8], !dbg !11
  store [8 x i8] %1, [8 x i8]* %0, align 1, !dbg !11
  %2 = bitcast [8 x i8]* %0 to i64*, !dbg !13
  %3 = load i64, i64* %2, align 1, !dbg !13
  ret i64 %3, !dbg !13
}

@eggyal
Copy link
Contributor

eggyal commented Jun 12, 2022

As for why to_array_inner puts the vector behind a pointer, the default rust abi passes vector types by-ref to ensure that changing target features doesn't change the abi.

It's interesting that the failure (in my example) required to_array_inner to be generic—that shouldn't affect the ABI of the monomorphized function, should it?

@bjorn3
Copy link
Member

bjorn3 commented Jun 12, 2022

Indeed. It shouldn't affect the ABI.

JohnTitor added a commit to rust-lang/glacier that referenced this issue Jun 24, 2022
@rust-lang-glacier-bot rust-lang-glacier-bot added the glacier ICE tracked in rust-lang/glacier. label Jun 24, 2022
@Alexendoo
Copy link
Member

Alexendoo commented Nov 28, 2022

The original no longer crashes since #96451, but #98016 (comment) still does

@JohnTitor
Copy link
Member

Triage: The above mentioned example now compiles fine with the latest nightly, I guess #107449 is related. Marking as E-needs-test.

@JohnTitor JohnTitor added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Feb 17, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 12, 2023
…li-obk

Test simd-wide-sum for codegen error

This adds the necessary test infrastructure to "build-pass" codegen tests, for the purpose of doing that for a single revision of a codegen test. When mir-opts are tested, the output may vary from the usual, and maybe for positive reasons... but we don't necessarily want to output such bad LLVMIR that LLVM starts crashing on it.

Currently when enabling MIR opts at higher levels this LLVMIR is still emitted, but it was previously disabled for getting in mir-opt's way and as this new revision without `// [mir-opt3]build-pass` would make it more likely to, I would like to not see the testing for the actual results regress again just because it was bundled with an ICE check as well.

This fixes rust-lang#98016
@bors bors closed this as completed in fc72c0f Jul 13, 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. A-mir-opt Area: MIR optimizations A-simd Area: SIMD (Single Instruction Multiple Data) 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. glacier ICE tracked in rust-lang/glacier. I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. PG-portable-simd Project group: Portable SIMD (https://github.com/rust-lang/project-portable-simd) 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.

6 participants