Skip to content

Conversation

sayantn
Copy link
Contributor

@sayantn sayantn commented Sep 10, 2025

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)

@rustbot
Copy link
Collaborator

rustbot commented Sep 10, 2025

r? @folkertdev

rustbot has assigned @folkertdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@folkertdev
Copy link
Contributor

I was under the impression that we want to phase out the usage of #[rustc_legacy_const_generics], not use it more?

@alexcrichton
Copy link
Member

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.

@heiher
Copy link
Contributor

heiher commented Sep 11, 2025

For LoongArch intrinsics that are still unstable, if all callers use const generics, should we remove #[rustc_legacy_const_generics]?

@Jamesbarford
Copy link
Contributor

For AArch64 to ease porting from c it would arguably make sense. It is on all of the stable intrinsics for AArch64

@Amanieu
Copy link
Member

Amanieu commented Sep 11, 2025

I think this really is a question for @rust-lang/lang: rustc_legacy_const_generics was originally added as a replacement for rustc_args_required_const. It allows functions with const generics to be called with the constants in argument position.

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:

  • It allow our intrinsics to match the signatures of C intrinsics which take compile-time constants as arguments.
  • Users find this way of passing constant parameters more ergonomic in practice than normal const generics which require a turbofish.

@sayantn
Copy link
Contributor Author

sayantn commented Sep 11, 2025

The question of whether we want to phase out #[rustc_legacy_const_generics] is important, but my motive for this PR was consistency across arch modules. All x86 intrinsics and most arm intrinsics already use it, and removing it is not possible without breakages. So I added these.

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 vec_extract::<_, 0>(a) to vec_extract(a, 0). I will update it, which means not all archs will get affected.

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.

@sayantn sayantn force-pushed the rustc-legacy-const-generics branch from 0c24485 to cac2005 Compare September 11, 2025 19:26
@sayantn
Copy link
Contributor Author

sayantn commented Sep 11, 2025

How is the error happening?? The diff shows that cacop has 1 const-generic parameter, but the CI error msg tells me it has 2! (I am aware that it recently got changed, I forgot to rebase the branch, why is it rebasing automatically?, or am I missing something?)

@sayantn sayantn force-pushed the rustc-legacy-const-generics branch from cac2005 to ff80eb1 Compare September 11, 2025 19:34
@traviscross
Copy link

traviscross commented Sep 11, 2025

Is this something that we would like to see more use of, or something that we eventually want to phase out?

The last time we discussed this was 2024-10-30, in the context of rust-lang/rust#130443, and the discussion went:

TC: This proposes to make an error out of:

std::arch::x86_64::_mm_blend_ps(loop {}, loop {}, foo::<3>());

...and similar cases that lean on legacy_const_generics.

This is not believed to break anything in practice and is being done for implementation reasons.

TC: This is marked as an easy decision for us. Do we agree?

Josh: We've thought about trying to make this a first-class feature. How diffcult are the bugs in question to fix?

CE: If we ever wanted this as a first-class feature, we'd need to tear this out anyway and reimplement it. What's in the compiler now cannot architecturally support extending this feature more generally.

NM: We may in fact want this first-class feature. But I would want it to be approached as an equality constraint being introduced, not a "rewrite".

CE: It could certainly be done; just not how it was done.

TC: This is now in FCP.

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.

@RalfJung
Copy link
Member

RalfJung commented Sep 15, 2025

Cc @rust-lang/wg-const-eval @rust-lang/project-const-generics

rustc_legacy_const_generics is an attribute decorating legacy functions to maintain backwards compatibility and should indeed not be added to new functions. That's why it has "legacy" in the name.

@RalfJung
Copy link
Member

How is the error happening?? The diff shows that cacop has 1 const-generic parameter, but the CI error msg tells me it has 2! (I am aware that it recently got changed, I forgot to rebase the branch, why is it rebasing automatically?, or am I missing something?)

Github CI merges your branch with the latest master branch and tests that combination.

@RalfJung
Copy link
Member

RalfJung commented Sep 15, 2025

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.)

@sayantn
Copy link
Contributor Author

sayantn commented Sep 15, 2025

In that case, should we introduce an fcw when using the c-style signature?

@RalfJung
Copy link
Member

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.

@sayantn
Copy link
Contributor Author

sayantn commented Sep 15, 2025

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

@RalfJung
Copy link
Member

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.

@sayantn
Copy link
Contributor Author

sayantn commented Sep 15, 2025

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.

@RalfJung
Copy link
Member

RalfJung commented Sep 15, 2025

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.

@RalfJung
Copy link
Member

RalfJung commented Sep 15, 2025

rust-lang/rust#82447 said (emphasis mine)

This allows previously stabilized functions in stdarch which use rustc_args_required_const to use const generics instead.

#1022 even said

Change the #[rustc_args_required_const(N)] attribute to #[rustc_legacy_const_generics(N)]. The N arguments to the attribute remains the same. This attribute allows legacy code to continue calling this function by passing constants as a parameter. The legacy support will likely be deprecated in Rust 2021.

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.)

@RalfJung
Copy link
Member

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.

@BoxyUwU
Copy link
Member

BoxyUwU commented Sep 15, 2025

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

@RalfJung
Copy link
Member

RalfJung commented Sep 15, 2025

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.

@Jamesbarford
Copy link
Contributor

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 rustc_legacy_const_generics?

@Amanieu
Copy link
Member

Amanieu commented Sep 15, 2025

I went back and read the discussion in rust-lang/rust#83167, which initially added support for rustc_legacy_const_generics. The consensus of the lang team at the time was in favor of not deprecating rustc_legacy_const_generics and extending it to include newly stabilized intrinsics for consistency with existing ones.

@BoxyUwU
Copy link
Member

BoxyUwU commented Sep 15, 2025

That seems to explicitly state that lang did not discuss whether to apply it to newly stabilized functions or not:

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

@RalfJung
Copy link
Member

RalfJung commented Sep 15, 2025

@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.)

@traviscross
Copy link

Nominated the questions raised by this in:

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 this pull request may close these issues.

10 participants