-
Notifications
You must be signed in to change notification settings - Fork 298
Add #[rustc_legacy_const_generics]
to all intrinsics with const-generic parameters
#1916
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
base: master
Are you sure you want to change the base?
Conversation
r? @folkertdev rustbot has assigned @folkertdev. Use |
I was under the impression that we want to phase out the usage of |
That was my impression as well, and assuming that's still the case I'd prefer to not add this feature to the wasm intrinsics and instead require true const generics be used. If the motivation is being able to copy C code into Rust I think that's also less of a motivation for the wasm target where that's not as prevalent as for x86 for example. |
For LoongArch intrinsics that are still unstable, if all callers use const generics, should we remove |
For AArch64 to ease porting from |
I think this really is a question for @rust-lang/lang: Is this something that we would like to see more use of, or something that we eventually want to phase out? The advantages of this over normal const generics are:
|
The question of whether we want to phase out But it seems like it doesn't like mixed generic and const-generic args, which is sad because that is a nice place to use it. It can transform But even if we want to phase it out, I would prefer if at least all ARM and loongarch intrinsics get it, just for the sake of consistency. |
0c24485
to
cac2005
Compare
How is the error happening?? The diff shows that |
cac2005
to
ff80eb1
Compare
The last time we discussed this was 2024-10-30, in the context of rust-lang/rust#130443, and the discussion went:
That is to say, we're trying to phase out as much as possible that relies on the current implementation and concept of this. At the same time, we're open to proposals for how we might approach this in a more appealing way. |
Cc @rust-lang/wg-const-eval @rust-lang/project-const-generics
|
Github CI merges your branch with the latest master branch and tests that combination. |
It seems there's quite a few recent PRs adding this attribute to new functions? That should not have happened, the entire idea was that this attribute should only be used where it is needed for backwards compatibility. It is a terrible hack, after all. @Amanieu please do not stabilize any further intrinsics with this attribute. If there is a need for more ergonomic const generic function, that should be done via a proper language feature, not by overextending the use of an ad-hoc legacy compatibility hack with (IIUC) deep architectural issues. (rust-lang/rust#130443 added more ad-hoc limitations to work around those issues, no idea how watertight that is.) |
In that case, should we introduce an fcw when using the c-style signature? |
So far I think there were no plans to remove the attribute from existing functions, just to stop adding it to new functions. It seems unlikely we'll be able to actually reject C-style uses of the old intrinsics, there's too much code out there using them. I think there's also a lingering hope that someone can make this into a nice proper language feature, but I have no idea what the main challenges for that are. |
Even if we want to not use this, we should provide this for all x86 and aarch64 functions at least, because a lot of intrinsics with this attribute has been stabilized, and not doing it anymore will just be inconsistent |
That was not the plan as I remember it from the last time this was discussed (but that has been years ago). The attribute is strictly a backwards compatibility mechanism. All docs and examples should use the new style. We don't usually make new APIs consistent with deprecated APIs. |
In context of this comment, it would be uninintuitive and confusing for users if all other avx512 intrinsics accept the C-style call, but only avx512fp16 doesn't. I understand your objection with using these in new code, but in this case, I think it should be it should be added. We shouldn't add it to new intrinsics (e.g. AMX, AVX10) though, that I agree on. |
None of the AVX512 intrinsics should ever have used this. :( And why would any user of AVX512 use the old syntax? It shouldn't be in any docs or examples. AVX512 intrinsics were stabilized just recently with Rust 1.89. Maybe we can still get away with removing this attribute there? Might be interesting to do a crater run. |
rust-lang/rust#82447 said (emphasis mine)
#1022 even said
That deprecation never happened, but it does seem quite clear that this was indeed not meant to be added to new functions. And yet we have a constant stream of new users over time (15babf5 landed just a few weeks after the initial port to const generics), most by you and @Jamesbarford. (To be clear you couldn't have known about this, though the "legacy" in the attribute does give a hint. We as a project should have caught this at some point before stabilization.) |
Turns out our docs do render this in a way that encourages people to use the c-style syntax: https://doc.rust-lang.org/nightly/core/arch/x86/fn._mm_shuffle_ps.html. |
That's unfortunate. We should definitely be rendering such functions as const-generic functions rather than the special desugaring. rustc_legacy_const_generics shouldn't be added to any newly added functions, we should go further and make it a hard error to rely on as a caller on newer editions. this syntax is more limited than actually providing the generic argument as a generic argument and is very much a backwards compatibility hack not a feature |
Apparently this was done because at the time, using the const generic version was a major MSRV bump. It's still true that the given MSRV in the docs is wrong if you actually do use the const generic syntax, but these days 1.51.0 is so old that maybe that's not such a big deal any more. |
When I did the port, I just carried over the intrinsics that were already there; I didn't introduce anything new. In hindsight, would that have been the right opportunity to drop |
I went back and read the discussion in rust-lang/rust#83167, which initially added support for |
That seems to explicitly state that lang did not discuss whether to apply it to newly stabilized functions or not:
|
@Amanieu good point, I didn't remember that t-lang left that open. However, as Boxy said, I can't see a t-lang decision in favor of using this for new intrinsics. @Mark-Simulacrum and @nikomatsakis expressed that this is their preference, but there was no team decision. (At least, not in that issue.) There also wasn't a team decision against using this though, so I clearly overstepped here. Sorry for that. (There was also some discussion whether the explicit const generic form should be allowed on stable, which just teetered off. That form has now been usable on stable for quite a while.) |
Nominated the questions raised by this in: |
This is very useful when writing intrinsic-heavy code. Adding it is not a semver hazard because the old style will still be allowed, but now the C-style code will also be allowed.
CCing arch-specific ppl to check that placement of the parameters are correct
LA64, LA32: @heiher
PPC: @lu-zero
S390X: @folkertdev
WASM: @alexcrichton
(I am tagging based on the commit history of the files I modified, pls cc if someone else is also involved)