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

simd_shuffle, simd_insert, simd_extract should use const generics #85229

Open
RalfJung opened this issue May 12, 2021 · 20 comments
Open

simd_shuffle, simd_insert, simd_extract should use const generics #85229

RalfJung opened this issue May 12, 2021 · 20 comments
Labels
A-const-generics Area: const generics (parameters and arguments) A-intrinsics Area: Intrinsics A-SIMD Area: SIMD (Single Instruction Multiple Data) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented May 12, 2021

simd_shuffle requires its last parameter, the array defining the shuffling, to be a constant:

emit_error!("shuffle index #{} is not a constant", arg_idx);

Currently (after #85110), this is enforced by an ad-hoc hack in compiler/rustc_mir/src/transform/lower_intrinsics.rs.

But I think it'd be better to instead change the type of simd_shuffle to make the shuffle array a const generic parameter. I am not doing this yet since const generic arrays require enabling an incomplete_feature which is not great, but once const generic arrays are sufficiently stable we could use them as a more principled way to ensure that simd_shuffle is called in the right way.

Update (2024-02-17): A const-generic version of simd_shuffle now exists, but using it in stdarch would require generic_const_exprs and that's too experimental for such uses.

Cc @rust-lang/project-const-generics

@workingjubilee
Copy link
Member

Would this enable using simd_shuffle as a single function without a suffix instead of calling e.g. simd_shuffle4?

@RalfJung
Copy link
Member Author

I had not thought about this... maybe? Do const generic support "dependent types" like this one?

fn simd_shuffle<const N: usize, const IDX: [u32; N]>(...)

@oli-obk
Copy link
Contributor

oli-obk commented May 12, 2021

I think the full const generics feature does support that, but there's still the open question on how we know whether the type is StructuralEq. For this specific case it is easy, because the array length does not influence that question, but we still need to figure out the details here

@nikomatsakis
Copy link
Contributor

cc @Amanieu -- is this is a case that should use the new attribute you added or have been adding?

@Amanieu
Copy link
Member

Amanieu commented May 13, 2021

Not really. simd_shuffle is a private intrinsic, so we don't need to worry too much about API user-friendlyness.

@oli-obk
Copy link
Contributor

oli-obk commented May 13, 2021

In that case... how about we change the const generic argument to be a slice?

Right now this

#![feature(const_generics)]
#![allow(incomplete_features)]

fn foo<const X: &'static [usize]>() {
    
}

fn main() {
    foo::<{&[1, 2, 3, 4]}>();
}

ICEs with

error: internal compiler error: compiler/rustc_mir/src/const_eval/mod.rs:160:14: cannot destructure constant Const { ty: &[usize], val: Value(ByRef { alloc: Allocation { bytes: [0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0], relocations: Relocations(SortedMap { data: [(Size { raw: 0 }, ((), alloc2))] }), init_mask: InitMask { blocks: [65535], len: Size { raw: 16 } }, size: Size { raw: 16 }, align: Align { pow2: 3 }, mutability: Not, extra: () }, offset: Size { raw: 0 } }) }

thread 'rustc' panicked at 'Box<Any>', /rustc/21e92b97309e15b16bc6b8dd4509d5e3ad4c430d/library/std/src/panic.rs:59:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

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.54.0-nightly (21e92b973 2021-05-12) running on x86_64-unknown-linux-gnu

note: compiler flags: -C embed-bitcode=no -C codegen-units=1 -C debuginfo=2 --crate-type bin

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

query stack during panic:
#0 [destructure_const] destructure constant
#1 [type_op_ascribe_user_type] evaluating `type_op_ascribe_user_type` `Canonical { max_universe: U0, variables: [CanonicalVarInfo { kind: Region(U0) }], value: ParamEnvAnd { param_env: ParamEnv { caller_bounds: [], reveal: UserFacing }, value: AscribeUserType { mir_ty: fn() {foo::<{&[1, 2, 3, 4]}>}, def_id: DefId(0:3 ~ playground[b426]::foo), user_substs: UserSubsts { substs: [Const { ty: &'static [usize], val: Unevaluated(Unevaluated { def: WithOptConstParam { did: DefId(0:6 ~ playground[b426]::main::{constant#0}), const_param_did: Some(DefId(0:4 ~ playground[b426]::foo::X)) }, substs: [], promoted: None }) }], user_self_ty: None } } } }`
end of query stack

but it seems totally straight forward to fix

@RalfJung
Copy link
Member Author

FWIW, there also currently seems to be no way to use any generic parameters in a const generic expression, even when indirecting via associated consts. Until that is resolved, using const generics for simd_shuffle (or simd_insert/simd_extract, which possibly should get the same treatment -- Cc #77477) is pretty much out of the question.

#![feature(const_generics, const_evaluatable_checked)]

pub fn print_const<const N: usize>() {
    println!("{}", N);
}

struct Const<T>(T);
impl<T> Const<T> {
    const N: usize = 5;
}

pub fn print_thing<T>() {
    print_const::<{Const::<T>::N}>()
}

This needs extra where bounds, which would be a breaking change to add to the stdarch API surface (I assume).

@oli-obk

This comment was marked as resolved.

@RalfJung

This comment was marked as resolved.

@oli-obk

This comment was marked as resolved.

@oli-obk

This comment was marked as resolved.

@RalfJung

This comment was marked as resolved.

@bjorn3 bjorn3 added A-const-generics Area: const generics (parameters and arguments) A-intrinsics Area: Intrinsics A-SIMD Area: SIMD (Single Instruction Multiple Data) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 15, 2021
@oli-obk

This comment was marked as resolved.

@RalfJung

This comment was marked as resolved.

@varkor

This comment was marked as resolved.

@luojia65

This comment was marked as resolved.

@workingjubilee

This comment was marked as resolved.

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 30, 2023
…gjubilee

Prototype using const generic for simd_shuffle IDX array

cc rust-lang#85229

r? `@workingjubilee` on the design

TLDR: there is now a `fn simd_shuffle_generic<T, U, const IDX: &'static [u32]>(x: T, y: T) -> U;` intrinsic that allows replacing

```rust
simd_shuffle(a, b, const { stuff })
```

with

```rust
simd_shuffle_generic::<_, _, {&stuff}>(a, b)
```

which makes the compiler implementations much simpler, if we manage to at some point eliminate `simd_shuffle`.

There are some issues with this today though (can't do math without bubbling it up in the generic arguments). With this change, we can start porting the simple cases and get better data on the others.
RalfJung pushed a commit to RalfJung/miri that referenced this issue Sep 30, 2023
Prototype using const generic for simd_shuffle IDX array

cc rust-lang/rust#85229

r? `@workingjubilee` on the design

TLDR: there is now a `fn simd_shuffle_generic<T, U, const IDX: &'static [u32]>(x: T, y: T) -> U;` intrinsic that allows replacing

```rust
simd_shuffle(a, b, const { stuff })
```

with

```rust
simd_shuffle_generic::<_, _, {&stuff}>(a, b)
```

which makes the compiler implementations much simpler, if we manage to at some point eliminate `simd_shuffle`.

There are some issues with this today though (can't do math without bubbling it up in the generic arguments). With this change, we can start porting the simple cases and get better data on the others.
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Oct 2, 2023
Prototype using const generic for simd_shuffle IDX array

cc rust-lang/rust#85229

r? `@workingjubilee` on the design

TLDR: there is now a `fn simd_shuffle_generic<T, U, const IDX: &'static [u32]>(x: T, y: T) -> U;` intrinsic that allows replacing

```rust
simd_shuffle(a, b, const { stuff })
```

with

```rust
simd_shuffle_generic::<_, _, {&stuff}>(a, b)
```

which makes the compiler implementations much simpler, if we manage to at some point eliminate `simd_shuffle`.

There are some issues with this today though (can't do math without bubbling it up in the generic arguments). With this change, we can start porting the simple cases and get better data on the others.
@RalfJung
Copy link
Member Author

RalfJung commented Feb 17, 2024

A const-generic version of simd_shuffle now exists. However, using it requires generic_const_exprs. From talking with @lcnr, this feature is still pretty unstable, so unfortunately stdarch cannot currently use simd_shuffle_generic.

It also needs adt_const_params since the array is passed as a slice. That's more mature though some thorny open questions make it unfit for stabilization (but I don't think those are a blocker for use in stdarch).

@RalfJung
Copy link
Member Author

My understanding is that the main complexity with generic_const_exprs is "what happens if const-eval fails". So would it make sense to try and carve out a min_generic_const_exprs of guaranteed-not-to-fail expressions? That would contain e.g. constructing an array and as casts, which would cover at least a large fraction of what simd_shuffle needs.

@lcnr
Copy link
Contributor

lcnr commented Feb 19, 2024

There are also quite a few issues wrt to generic consts in where-clauses and some other less fundamental implementation challenges. While it hasn't been updated recently as there hasn't been any focussed work on gce, here's a list of the known issues https://github.com/rust-lang/project-const-generics/issues?q=is%3Aissue+is%3Aopen+label%3AA-generic-exprs+label%3AC-design-docs+

@RalfJung RalfJung changed the title simd_shuffle should use const generics simd_shuffle, simd_insert, simd_extract should use const generics Feb 20, 2024
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
Prototype using const generic for simd_shuffle IDX array

cc rust-lang/rust#85229

r? `@workingjubilee` on the design

TLDR: there is now a `fn simd_shuffle_generic<T, U, const IDX: &'static [u32]>(x: T, y: T) -> U;` intrinsic that allows replacing

```rust
simd_shuffle(a, b, const { stuff })
```

with

```rust
simd_shuffle_generic::<_, _, {&stuff}>(a, b)
```

which makes the compiler implementations much simpler, if we manage to at some point eliminate `simd_shuffle`.

There are some issues with this today though (can't do math without bubbling it up in the generic arguments). With this change, we can start porting the simple cases and get better data on the others.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Prototype using const generic for simd_shuffle IDX array

cc rust-lang/rust#85229

r? `@workingjubilee` on the design

TLDR: there is now a `fn simd_shuffle_generic<T, U, const IDX: &'static [u32]>(x: T, y: T) -> U;` intrinsic that allows replacing

```rust
simd_shuffle(a, b, const { stuff })
```

with

```rust
simd_shuffle_generic::<_, _, {&stuff}>(a, b)
```

which makes the compiler implementations much simpler, if we manage to at some point eliminate `simd_shuffle`.

There are some issues with this today though (can't do math without bubbling it up in the generic arguments). With this change, we can start porting the simple cases and get better data on the others.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) A-intrinsics Area: Intrinsics A-SIMD Area: SIMD (Single Instruction Multiple Data) 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

9 participants