Compiler bug in simd intrinsics for usize types

[dyv]

As far as I can tell this bug only exists when performing a simd gather instruction on a slice of usize. If the slice is instead typed as u64 or u32 this code succeeds.

Code

#![feature(portable_simd)]
use core_simd::Simd;

fn main() {
    let indexes: [usize; 4] = [0, 1, 2, 3];
    let simd_indexes: Simd<usize, 4> = Simd::from_array(indexes);
    let slice: [usize; 4] = [10, 11, 12, 13];
    let result = Simd::gather_or_default(&slice, simd_indexes);
    assert_eq!(result, Simd::from_array([10, 11, 12, 13]));
}

Cargo.toml

[package]
name = "simd-repro"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
core_simd = { git = "https://github.com/rust-lang/portable-simd", rev = "c2f59483f96cf1ab1e92cf10e0f9094432a8374c" }

Meta

rustc --version --verbose:

rustc 1.57.0-nightly (ac2d9fc50 2021-09-21)
binary: rustc
commit-hash: ac2d9fc509e36d1b32513744adf58c34bcc4f43c
commit-date: 2021-09-21
host: x86_64-unknown-linux-gnu
release: 1.57.0-nightly
LLVM version: 13.0.0

Error output

dylan@lion:~/src/simd-repro$ RUST_BACKTRACE=1 cargo run
   Compiling simd-repro v0.1.0 (/home/dylan/src/simd-repro)
thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', compiler/rustc_codegen_llvm/src/intrinsic.rs:1194:76
stack backtrace:
   0: rust_begin_unwind
             at /rustc/ac2d9fc509e36d1b32513744adf58c34bcc4f43c/library/std/src/panicking.rs:517:5
   1: core::panicking::panic_fmt
             at /rustc/ac2d9fc509e36d1b32513744adf58c34bcc4f43c/library/core/src/panicking.rs:103:14
   2: core::panicking::panic
             at /rustc/ac2d9fc509e36d1b32513744adf58c34bcc4f43c/library/core/src/panicking.rs:50:5
   3: rustc_codegen_llvm::intrinsic::generic_simd_intrinsic::llvm_vector_str
   4: rustc_codegen_llvm::intrinsic::generic_simd_intrinsic
   5: rustc_codegen_llvm::intrinsic::<impl rustc_codegen_ssa::traits::intrinsic::IntrinsicCallMethods for rustc_codegen_llvm::builder::Builder>::codegen_intrinsic_call
   6: rustc_codegen_ssa::mir::intrinsic::<impl rustc_codegen_ssa::mir::FunctionCx<Bx>>::codegen_intrinsic_call
   7: rustc_codegen_ssa::mir::block::<impl rustc_codegen_ssa::mir::FunctionCx<Bx>>::codegen_terminator
   8: rustc_codegen_ssa::mir::codegen_mir
   9: rustc_codegen_ssa::base::codegen_instance
  10: rustc_codegen_llvm::base::compile_codegen_unit::module_codegen
  11: rustc_query_system::dep_graph::graph::DepGraph<K>::with_task
  12: rustc_codegen_llvm::base::compile_codegen_unit
  13: rustc_codegen_ssa::base::codegen_crate
  14: <rustc_codegen_llvm::LlvmCodegenBackend as rustc_codegen_ssa::traits::backend::CodegenBackend>::codegen_crate
  15: rustc_interface::queries::Queries::ongoing_codegen
  16: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::enter
  17: rustc_span::with_source_map
  18: scoped_tls::ScopedKey<T>::set
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.57.0-nightly (ac2d9fc50 2021-09-21) running on x86_64-unknown-linux-gnu

note: compiler flags: -C embed-bitcode=no -C debuginfo=2 -C incremental --crate-type bin

note: some of the compiler flags provided by cargo are hidden

query stack during panic:
end of query stack
error: could not compile `simd-repro`

[workingjubilee]

    // FIXME: use:
    //  https://github.com/llvm-mirror/llvm/blob/master/include/llvm/IR/Function.h#L182
    //  https://github.com/llvm-mirror/llvm/blob/master/include/llvm/IR/Intrinsics.h#L81
    fn llvm_vector_str(elem_ty: Ty<'_>, vec_len: u64, no_pointers: usize) -> String {
        let p0s: String = "p0".repeat(no_pointers);
        match *elem_ty.kind() {
            ty::Int(v) => format!("v{}{}i{}", vec_len, p0s, v.bit_width().unwrap()),
            ty::Uint(v) => format!("v{}{}i{}", vec_len, p0s, v.bit_width().unwrap()),
            ty::Float(v) => format!("v{}{}f{}", vec_len, p0s, v.bit_width()),
            _ => unreachable!(),
        }
    }

Here we attempt to invoke .bit_width() on Uints and Ints, which may be Usize or Isize, which do not have a specified bit width and so return the None value which we gleefully unwrap. We can invoke v.normalize(target_width).bit_width().unwrap() instead in these cases, OR change this to use the mentioned LLVM bindings (though any such effort should really reference a tag in https://github.com/rust-lang/llvm-project instead). Though: I am surprised that these integers are not already normalized at this point.

[gcohara]

I'm happy to work on this.
@rustbot claim

[gcohara]

I found this comment in intrinsic.rs:

rust/compiler/rustc_codegen_llvm/src/intrinsic.rs

Lines 850 to 856 in 15d9ba0

	let m_len = match in_ty.kind() {
	// Note that this `.unwrap()` crashes for isize/usize, that's sort
	// of intentional as there's not currently a use case for that.
	ty::Int(i) => i.bit_width().unwrap(),
	ty::Uint(i) => i.bit_width().unwrap(),
	_ => return_error!("`{}` is not an integral type", in_ty),
	};

It might be intentional, but maybe an ICE isn't the best way to go. Should some kind of error be added in?
Otherwise I'm not really sure what width it should be normalised to. I did think the most obvious would be the target arch's width but I'm not really sure how to get that within this function since there doesn't seem to be a target specification within scope.

[workingjubilee]

I believe that comment applies only for handling simd_select_bitmask. I do not see how the reasoning expressed there applies to simd_gather or simd_scatter, as it is (admittedly unusual) but valid to want to gather or scatter slices of those.

Ideally some refactoring could happen to make it clearer such reasoning is isolated from the rest of these intrinsics.
cc @calebzulawski

[calebzulawski]

Yes, that only applies to bitmasks, which cannot be usize/isize.

[workingjubilee]

Otherwise I'm not really sure what width it should be normalised to. I did think the most obvious would be the target arch's width but I'm not really sure how to get that within this function since there doesn't seem to be a target specification within scope.

And it should be normalized to the target width, yes.
There should be relevant functions, types, and methods for doing so either already present in the imports from use rustc_target::abi::{} or you can find them elsewhere in rustc_target. I believe you want to be calling https://doc.rust-lang.org/nightly/nightly-rustc/rustc_target/abi/trait.HasDataLayout.html#tymethod.data_layout which will yield https://doc.rust-lang.org/nightly/nightly-rustc/rustc_target/abi/struct.TargetDataLayout.html and then all the information should be there.

Feel free to r? me on your PR, including a draft.

[dyv]

Since there is a bit of chatter here on the use cases for performing a scatter or gather for indexes, I can give a little bit of context, since I do think it is somewhat informative.

In this case I am working on a columnar store. You can kind of think of the use case as a secondary index over columnar data. For just a single primary index, you have a list of row_ids Simd<usize, LANES> and you can perform a simd gather instruction to get all of the values at those indexes. However, if you have a secondary index the simd gather from that secondary index needs to produce a Simd<usize, LANES>, so that can be passed directly into the second of the gather functions over the primary keys. In this way you go from needed 2*LANES loads to perform a secondary index lookup over data to only needing 2 simd operations.

Alternatively it would be nice if the Simd libraries were updated to perform gathers using indexes as Simd<u32, LANES> or Simd<u64, LANES>. That would dodge this issue altogether and it would mean less copies in our columnar store. Many times columnar stores you keep offsets as u32 (or even smaller) to keep things compressed. And it is nice to be able to act directly on those indices rather than performing intermediate conversions to usize.