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

ICE with unusual vector sizes #53

Closed
calebzulawski opened this issue Jan 1, 2021 · 6 comments · Fixed by rust-lang/rust#80652
Closed

ICE with unusual vector sizes #53

calebzulawski opened this issue Jan 1, 2021 · 6 comments · Fixed by rust-lang/rust#80652

Comments

@calebzulawski
Copy link
Member

When trying to make a vector with only 3 lanes, the following occurs:

error: internal compiler error: compiler/rustc_codegen_llvm/src/context.rs:448:55: unknown intrinsic 'llvm.ceil.v3f64'

A couple options:

  • Only support power-of-two vector lengths. This is probably fine (at least for the MVP) but would probably require a new attribute for restricting the const generic implementation.
  • Patch rustc to use the next power of two for ceil etc. Most other ops seem to work fine with unusual vector sizes.
@bjorn3
Copy link
Member

bjorn3 commented Jan 1, 2021

Only support power-of-two vector lengths.

Cranelift only has support for power-of-two lengths.

This is probably fine (at least for the MVP) but would probably require a new attribute for restricting the const generic implementation.

Or the layout computation could disallow non power-or-two lengths for #[repr(simd)] types.

Patch rustc to use the next power of two for ceil etc.

For at least x86, non power-of-two lengths are not very efficient. Each load and store would need to use a single instruction for every lane.

@calebzulawski
Copy link
Member Author

For at least x86, non power-of-two lengths are not very efficient. Each load and store would need to use a single instruction for every lane.

x86 does have masked loads and stores but in my experience those are pretty terrible as well.

I do like the idea of adding the check to the repr attribute.

I just encountered another related issue with vectors that are powers of two but too large, such as 128 usizes. It doesn't ICE but it does error. I think this should be allowed since it's trivial to compose smaller vectors. Does cranelift support that?

@bjorn3
Copy link
Member

bjorn3 commented Jan 1, 2021

Cranelift uses 16bit lane lengths, but there is a limit on the max lane size that I couldn't quickly figure out. Cranelift theoretically has support for splitting vectors operations when the vectors are too big, but the new backends don't support it yet and for the old backends, I believe it wasn't reliable. I think support should be added in the future though.

@workingjubilee
Copy link
Member

I believe our MVP can focus on 2.pow(N) lanes for now without negative consequences, but an actual extension where we simply politely ignore the 8th lane is more elegant, yes. If Cranelift supports up to 65536 lanes then I believe it will satisfy any future lane lengths we care about.

@bjorn3
Copy link
Member

bjorn3 commented Jan 6, 2021

Cranelift supports any power of two lane length of up to u16::MAX lanes. (so it actually only supports up to 2^15 lanes) Cranelift stores the log2 of the lane length in the Type struct.

bors added a commit to rust-lang-ci/rust that referenced this issue Feb 7, 2021
Improve SIMD type element count validation

Resolves rust-lang/portable-simd#53.

These changes are motivated by `stdsimd` moving in the direction of const generic vectors, e.g.:
```rust
#[repr(simd)]
struct SimdF32<const N: usize>([f32; N]);
```

This makes a few changes:
* Establishes a maximum SIMD lane count of 2^16 (65536).  This value is arbitrary, but attempts to validate lane count before hitting potential errors in the backend.  It's not clear what LLVM's maximum lane count is, but cranelift's appears to be much less than `usize::MAX`, at least.
* Expands some SIMD intrinsics to support arbitrary lane counts.  This resolves the ICE in the linked issue.
* Attempts to catch invalid-sized vectors during typeck when possible.

Unresolved questions:
* Generic-length vectors can't be validated in typeck and are only validated after monomorphization while computing layout.  This "works", but the errors simply bail out with no context beyond the name of the type.  Should these errors instead return `LayoutError` or otherwise provide context in some way?  As it stands, users of `stdsimd` could trivially produce monomorphization errors by making zero-length vectors.

cc `@bjorn3`
@calebzulawski
Copy link
Member Author

Closed in rust-lang/rust#80652.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants