Use const generics for stdarch intrinsics

[Amanieu]

Background

rust-lang/stdarch#248

core::arch contains architecture-specific vendor intrinsics, some of which have arguments that are required to be compile-time constants. This is tricky because LLVM will often assert/ICE if a constant is not passed into the LLVM intrinsic at the LLVM IR level. This was previously handled by using a match statement to manually monomorphize the intrinsic:

#[inline]
#[target_feature(enable = "sse")]
#[cfg_attr(test, assert_instr(shufps, mask = 3))]
#[rustc_args_required_const(2)]
#[stable(feature = "simd_x86", since = "1.27.0")]
pub unsafe fn _mm_shuffle_ps(a: __m128, b: __m128, mask: i32) -> __m128 {
    let mask = (mask & 0xFF) as u8;

    macro_rules! shuffle_done {
        ($x01:expr, $x23:expr, $x45:expr, $x67:expr) => {
            simd_shuffle4(a, b, [$x01, $x23, $x45, $x67])
        };
    }
    macro_rules! shuffle_x67 {
        ($x01:expr, $x23:expr, $x45:expr) => {
            match (mask >> 6) & 0b11 {
                0b00 => shuffle_done!($x01, $x23, $x45, 4),
                0b01 => shuffle_done!($x01, $x23, $x45, 5),
                0b10 => shuffle_done!($x01, $x23, $x45, 6),
                _ => shuffle_done!($x01, $x23, $x45, 7),
            }
        };
    }
    macro_rules! shuffle_x45 {
        ($x01:expr, $x23:expr) => {
            match (mask >> 4) & 0b11 {
                0b00 => shuffle_x67!($x01, $x23, 4),
                0b01 => shuffle_x67!($x01, $x23, 5),
                0b10 => shuffle_x67!($x01, $x23, 6),
                _ => shuffle_x67!($x01, $x23, 7),
            }
        };
    }
    macro_rules! shuffle_x23 {
        ($x01:expr) => {
            match (mask >> 2) & 0b11 {
                0b00 => shuffle_x45!($x01, 0),
                0b01 => shuffle_x45!($x01, 1),
                0b10 => shuffle_x45!($x01, 2),
                _ => shuffle_x45!($x01, 3),
            }
        };
    }
    match mask & 0b11 {
        0b00 => shuffle_x23!(0),
        0b01 => shuffle_x23!(1),
        0b10 => shuffle_x23!(2),
        _ => shuffle_x23!(3),
    }
}

The downside of this approach is that it generates huge MIR and severely bloats the size of libcore to the point where it actually affects compilation time.

The #[rustc_args_required_const] attribute was used to restrict arguments to constants so that we could eventually replace this with const generics. Note that it effectively only acts as a hard lint, the match is still needed to manually monomorphize the LLVM intrinsic.

New const generics support

rust-lang/stdarch#1022

All the intrinsics in stdarch have been converted to use the newly stabilized const generics:

#[inline]
#[target_feature(enable = "sse")]
#[cfg_attr(test, assert_instr(shufps, mask = 3))]
#[rustc_legacy_const_generics(2)]
#[stable(feature = "simd_x86", since = "1.27.0")]
pub unsafe fn _mm_shuffle_ps<const mask: i32>(a: __m128, b: __m128) -> __m128 {
    static_assert!(mask: i32 where mask >= 0 && mask <= 255);
    simd_shuffle4(
        a,
        b,
        [
            mask as u32 & 0b11,
            (mask as u32 >> 2) & 0b11,
            ((mask as u32 >> 4) & 0b11) + 4,
            ((mask as u32 >> 6) & 0b11) + 4,
        ],
    )
}

To preserve backwards compatibility with the already stabilized intrinsics using #[rustc_args_required_const], a new attribute #[rustc_legacy_const_generics] was added in #82447 which rewrites function calls of the form func(a, b, c) to func::<{b}>(a, c).

This new attribute is not intended to ever be stabilized, it is only intended for use in stdarch as a replacement for #[rustc_args_required_const].

#[rustc_legacy_const_generics(1)]
pub fn foo<const Y: usize>(x: usize, z: usize) -> [usize; 3] {
    [x, Y, z]
}

fn main() {
    assert_eq!(foo(0 + 0, 1 + 1, 2 + 2), [0, 2, 4]);
    assert_eq!(foo::<{1 + 1}>(0 + 0, 2 + 2), [0, 2, 4]);
}

Open questions/issues

• #[rustc_args_required_const] allowed constant expressions derived from generic arguments in the caller, but this is not allowed with const generics. This technically makes this a breaking change.
• Another potential breaking change is that the intrinsics now use a post-monomorphization static_assert to validate constant arguments. Previously, intrinsics would either use runtime asserts or simply pick an arbitrary default behavior for out-of-range constants.
• Do we want to force all callers to use the const generics syntax in the 2021 edition?
  • Pros:
    • It makes it clearer that these intrinsics expect a constant as an argument.
    • Intrinsics are rendered with the const generics syntax in rustdoc.
  • Cons:
    • We will still need to support the #[rustc_legacy_const_generics] argument rewriting anyways for 2015/2018 code.
    • Intrinsics no longer have the same function signature as the original C vendor intrinsics.

[bstrie]

Apparently the implementation of this so far reduces the size of libcore from 58MB to 40MB, which is still pretty substantial. Is this an acceptable size? Is there reason to think this will shrink further in the future?

[Amanieu]

All of the functions have been converted to const generics, so I don't expect any further shrinkage.

[tmiasko]

The libcore_arch.rlib is around 17 MB now.

[lqd]

That size comment was about the size of libcore_arch.rlib, not libcore per se, and was before we had converted the huge AVX512 intrinsics: the latest size I saw during conversion was around 16MB.

[nikomatsakis]

In the Edition 2021 meeting, @m-ou-se mentioned that the libs team had this idea as a compromise:

• New edition only accepts proper rust syntax (2nd one)
• But produce a warning with a machine applicable fix from the 1st one

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting. In today's discussion, there was some debate about this migration, particularly since we already need to maintain the rustc_legacy_const_generics mechanism indefinitely to support existing code.

We would, ideally, love to get some feedback from existing users of the SIMD intrinsics, perhaps by asking the maintainers of crates that use intrinsics such as _mm_shuffle_ps; perhaps a search through crates.io for occurrences of any of the intrinsics requiring a const parameter could turn up such usage. We'd also love to get some feedback from developers of tools that operate on Rust code, regarding the difficulty of supporting this.

On balance, we may potentially want to support this on an ongoing basis, in order to match the vendor intrinsics documentation, and in particular the argument order. There was concern that people reading such code would find that code harder to follow if the arguments in a different order.

The possibility also came up (though there was not consensus on it) of providing a macro for each such intrinsic, and rustfix-ing code using the function to use the macro rather than using const generics syntax.

[bstrie]

A macro seems like the most principled way of doing this, especially considering that these are also implemented via macros in C. It would resolve the tension between wanting these APIs to look idiomatic vs wanting them to exactly mirror the vendor documentation. That sure would be a whole lot of macros, though...

[Mark-Simulacrum]

I personally think that we should use the 'legacy' attribute on all intrinsics (both existing and potentially ones added in the future) that need the compile-time generic argument.

The intrinsics API has been designed to match the platform API (in particular, usually the one available in C), and the choice was explicitly made to note deviate from the vendor intrinsics in naming or calling convention. It would be odd, IMO, to change that choice in the case of the subset of intrinsics which need compile-time arguments. I think that these intrinsics are already pretty hard to read and changing the argument order by pulling out compile-time arguments is likely to be relatively painful for users, particularly if trying to align C and Rust code.

And since we have to support the legacy syntax on some intrinsics, I think it would be preferable to support it on all intrinsics, to avoid some having one API and some having a different API. Splitting the intrinsics into two groups just based on their stabilization date seems quite strange and not easily learnable by users.

I also think adding macros here may not necessarily be a bad idea, but it would be a lot of macros, and these intrinsics are not all that different compared to other intrinsics from the caller's perspective AFAIK -- Intel just made the choice to not embed the compile-time constant in the name of the intrinsic, as is done for some of the other implicit constants. Deviating for some intrinsics to macros seems like it needlessly complicates the interface; and moving all intrinsics to macros feels pretty obviously the wrong choice, and not very Rust-y.

It was proposed that we deprecate (i.e., with a warning) the non-const-generic invocation (i.e. where all arguments are given inside the parens, including the compile-time ones), which I think would cause needless churn -- there's not really anything wrong with the way these intrinsics have been called up until now.

Plus, I think there's some possibility that we may eventually want to add support for passing const values as function parameters (which would desugar to generics), similar to how we have added foo: impl Trait -- if we do, then there would be a potentially much smaller surface area of 'special case' added by these legacy attributes, and potentially they'd be entirely removable. While I don't think we should try to design that now, I do think it's meaningful to say that the idea is not entirely unlikely.

One thing I did think of is that we may want to consider that eventually the function-argument position for const generics may want to gain a const { } block around it to avoid an implicit promotion context that may otherwise exist. That may mitigate some of the concerns we had around tooling supporting in the meeting.

[bstrie]

there's not really anything wrong with the way these intrinsics have been called up until now.

That may not be entirely true; I have heard that the magical nature of these functions is a pain point for the IDE use case.

[nikomatsakis]

I'd like to hear some folks who are creating tooling weigh in on that point. I was saying in the meeting that it seemed like two sets of people are affected by this:

• those using the intrinsics;
• tool makers, who have to consider all the edge cases of the language.

For most other folks, it hardly matters.

One other thing to consider is that we may want to support this use case natively at some point. I could imagine supporting something like

fn shuffle(x: u32, const y: u32) { }

or -- perhaps closer to what is being proposed here --

fn shuffle<const N:usize>(x: u32, y: =N) { }

Obviously i don't want to block progress on some wild new features, it's just that I can imagine going the other way, and making this feature be something that people can use in all APIs to improve inference and make them more ergonomic.

[nikomatsakis]

That said, having let this sit, I'm somewhat inclined to get stricter in Rust 2021, and we could always get loose again. But I would very much like to hear from affected users to have a sense of whether it will be a pain. I agree with @Mark-Simulacrum that trying to make the intrinsics match the documentation precisely was a design goal, and it does seem like a bit of a shame to give it up, especially given that we have to have the backwards compatibility code anyway. (On the other hand, if we get stricter in Rust 2021+, we may find we can remove that code at some point.)

[luojia65]

This is remarkably convenient in RISC-V's vector V instruction set. V set contains tons of types and functions where it would not be clear to just make a huge list for all of them.

[nikomatsakis]

@luojia65 can you elaborate on that? Do you mean specifically that being able to invoke the functions without using "turbofish syntax" is remarkable convenient?

[Lokathor]

Hello. Maintainer of safe_arch. It covers all safe-able Stable intrinsics. As people have noted, many functions have been exposed via macro_rules.

Just this past weekend I've been updating it to use min_const_generics.

My impression is that the basic code readability is going down the more I convert and update things to the const generic version.

[nikomatsakis]

@Lokathor and therefore you might prefer to keep the "in-place call" notation?

[Lokathor]

Yeah.

The big problem is that turbofish is cute and I'll fight anyone who tries to remove it, but also it's cute because it's rare. Having to put a turbofish on every single call to a function quickly becomes a chore, and so far I'm not even writing "real usage code", just converting doc tests to integration tests.

That said I will continue to convert the crate to min_const_generics to not use macro_rules because for the purposes of wrapping unsafe code a function call is a much sturdier thing to trust than a macro_rules expansion (even if you think you probably really locked down that macro).

However, the long term "what makes const generics good to use with function calls?" solution will essentially require that the const value be passable within the parameters instead of as a turbofish. Anything less just reads super poorly for functions in a way that doesn't happen as badly for types because types can infer or be trivially aliased, and functions not so much.

[nikomatsakis]

However, the long term "what makes const generics good to use with function calls?" solution will essentially require that the const value be passable within the parameters instead of as a turbofish. Anything less just reads super poorly for functions in a way that doesn't happen as badly for types because types can infer or be trivially aliased, and functions not so much.

This is roughly my opinion. I think we're going to want this feature sooner or later in some form. That is partly why I'm ok leaving this hack in for the time being.

[Mark-Simulacrum]

The language team discussed this in the last triage meeting (8 days ago). The feeling was that additional feedback is going to be hard to source without active investment, and the status quo is quite reasonable to leave until the next edition (e.g., 2024). Indeed, some people in the room (e.g., me) felt that the legacy syntax is better, and should be preferred; feedback since then from @Lokathor continues to agree with this.

So, the current (meeting) consensus in the lang team was that we'd un-nominate this issue for the time being. We didn't feel there was a need to take action before the 2021 edition to deprecate and/or remove this pattern; it seemed fine to continue with the status quo.

However, one thing I'm not realizing we did not discuss, and seems like we may want an FCP for, is whether the legacy attribute should be added to not-yet stabilized intrinsics, or kept strictly to things that we cannot change due to backwards compatibility concerns. There was some discussion a few weeks back on this, but I don't believe there were firm conclusions from that discussion (even in the room). @rust-lang/lang -- I think an async FCP may be good here, but we may want some discussion as to which of these options to prefer. Libs may also have an opinion here.

1. Preserve exact status-quo. Do not stabilize any new intrinsics/functions using the legacy attribute, which enables both const-generic call syntax (i.e., turbofish) and parameter-as-const-value call syntax.
2. Newly added intrinsics stabilized with the legacy attribute.

(1) is, in my opinion, not really coherent. We don't gain anything by refusing to add more legacy attributes -- there are already 1424 instances of the legacy attribute on master stdarch (though not all may be on stable functions, I'd need to do more work to check that). We do gain overall cohesion with (2), though, as users can always use the same syntax to call any intrinsic. I wouldn't suggest expanding that to more general std/core functions (e.g., iterator methods) at this time, but intrinsics are always a bit special and IMO it makes sense to have a uniform calling syntax there (well, two alternatives, but still).

[Lokathor]

I agree with (2). All of stdarch should use the same legacy attribute for consistency across the module. As was mentioned earlier in the thread: in a few years no one will care what historical order the intrinsics stabilized in as an explanation for an inconsistency, they will just see the inconsistency and consider it an annoying weird rule.

[nikomatsakis]

I also agree with (2), for the obvious reasons.

[nikomatsakis]

We've decided to continue allowing these intrinsics to be called with the old syntax in the 2021 edition. Should we also allow calling these intrinsics with the const generics syntax?

I'm inclined not to support it, to give ourselves more room to design a first-class feature around this.

[RalfJung]

I was briefly very confused by the term "intrinsics" here. These are not intrinsics in the rustc sense, are they? They are just regular functions? The true intrinsics are an internal implementation detail of stdarch, not visible to the user? Might be better to not call them "intrinsics" then in the rustc issue tracker...

[RalfJung]

Another example of code that compiles on stable but not on nightly:

use std::arch::x86_64::*;
use std::mem::transmute;

#[target_feature(enable = "sse2")]
pub unsafe fn sse_constant_shift<const N: i32>(a: __m128i) {
    let x = 2;
    let r = _mm_slli_epi16(a, 4+N);
}

[bstrie]

I'm inclined not to support it, to give ourselves more room to design a first-class feature around this.

In particular it would be consistent for a hypothetical const-param-in-argument-position to act similarly to impl-trait-in-argument-position, where fn foo(x: impl Bar) can't be invoked as foo::<Qux>() (arguably this restriction is controversial, but it can always be lifted later).

[Lokathor]

I was briefly very confused by the term "intrinsics" here.

All the rust docs call them arch intrinsics, cpu intrinsics, or vendor intrinsics. It's simply the term for them by now, that train has long left the station.

[RalfJung]

"arch intrinsics" is much less confusing than just plain "intrinsics". :)
I assume when reading the issue front-to-back this is clear from context, but when coming back to this discussion after weeks and just reading the last 2 or 3 messages, being more explicit helps.

[Amanieu]

Another example of code that compiles on stable but not on nightly:

We checked for this in crater and only 2 crates were affected. Both have since been fixed to work around this issue.

[RalfJung]

Ah, and I guess those failures would be caught even by the check-only run.

[Mark-Simulacrum]

Should we change rustdoc to render the const generics differently when #[rustc_legacy_const_generics] is present?

I think there should be some indication that the "legacy" call signature is allowed. Perhaps even defaulting to that rendering would be preferable, given that it matches both (a) historical practice in Rust and (b) the convention in C or C++ when invoking these intrinsics.

@rust-lang/rustdoc, given that this is in 1.54 which ships in 2 weeks, do we think we can create a patch that adds some indication to the rendering by then? Right now a page for these intrinsics (e.g., https://doc.rust-lang.org/beta/core/arch/x86_64/fn._mm256_mask_extracti32x4_epi32.html) doesn't give any indication that both calling modes are supported, which seems quite confusing for any user coming across a call in the old convention.

@Amanieu Is it intentional that e.g. https://doc.rust-lang.org/stable/core/arch/x86_64/fn._mm256_mask_extracti32x4_epi32.html references "imm8" as the name of the parameter, but the const generic has now changed this to IMM1? I am surprised to see us rewriting Intel's docs and names, given that the idea with x86_64 intrinsics was to try and stay "as close as possible" to the canonical names from the intel docs...

[Manishearth]

do we think we can create a patch that adds some indication to the rendering by then?

Maybe ... what kind of UI do you expect here?

[Mark-Simulacrum]

I think there's a couple options:

1. Render them as-if they were still in the old format (i.e., re-inline the const generic, perhaps as something like fn foo(const name: ty) rather than fn foo<const name: ty>()).
2. Add a box like the unstable/target feature/etc with a note like "this function is also callable with the generic inline with the arguments"
   • it's not very clear how to indicate which argument with this form. Maybe something like "with the const generic as the 5th argument" could work, though.

I'm likely at least a little biased, but since I recall some expression from some members of @rust-lang/lang in meetings on this of potentially wanting to add the "not genericy" name as a stable alternative/feature instead of the #[rustc_legacy_const(...)] attributes, I'd probably roughly suggest option one. Maybe folks on T-libs or T-lang have opinions though.

[Manishearth]

it's not very clear how to indicate which argument with this form. Maybe something like "with the const generic as the 5th argument" could work, though.

a really cool way to do this would be to have this annotation, and you can click it to swap the signature.

[GuillaumeGomez]

it's not very clear how to indicate which argument with this form. Maybe something like "with the const generic as the 5th argument" could work, though.

a really cool way to do this would be to have this annotation, and you can click it to swap the signature.

Let's try to keep the UI as simple as possible (even though that sounds good). 😆

Once we've reached a consensus on the what the UI should look like, I can send a PR if you want.

[lqd]

Note: Intel only uses the imm8 name, and documents its acceptable width in the documentation (it's not clearly spelled out there, and has to be inferred from text or the pseudo-code), and explicitly in the guide's xml data like so: <parameter type="int" varname="imm8" etype="IMM" immwidth="1"/>.

[Mark-Simulacrum]

I was not aware of that context, thanks @lqd -- in that case I think the names seem like an improvement indeed. Maybe we can put a quick section in the root (e.g., https://doc.rust-lang.org/nightly/core/arch/x86_64/index.html and https://doc.rust-lang.org/nightly/core/arch/x86/index.html) that mentions that the names of immediate parameters describe their widths (imm1 -> 1 bit, etc.)? Could help readers who are broadly unfamiliar otherwise.

I would still like to see improvements to the rendered documentation, though.

[newpavlov]

Rust 1.54 now supports both foo(a, b, c) and foo::<c>(a, b). Was it an explicit decision to stabilize the latter? Judging by @nikomatsakis' comments I was expecting that only the former will be stable for the time being. Release notes do not contain any mention of this. I hope it was not an accidental stabilization...

[kazcw]

The current documentation is problematic. I was just bitten (cryptocorrosion/cryptocorrosion#48) by using a SIMD fn which the docs say has been stable since 1.27, but the documented syntax is actually much newer than that; only the original syntax, which is not referenced in the documentation, has that level of compatibility.

[Amanieu]

I think we should at the very least change rustdoc to render the function using the old syntax instead of the new one.

@GuillaumeGomez How difficult do you think this would be? We essentially need generics to appear as normal arguments in rustdoc for functions marked with #[rustc_legacy_const_generics].

[GuillaumeGomez]

Without checking too much, I think it wouldn't be too complicated. To be confirmed. I'll work on it in the following days.