-
Notifications
You must be signed in to change notification settings - Fork 14k
rustc_target: aarch64: Remove deprecated FEAT_TME #148951
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: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
688954b to
8f7dadd
Compare
This comment has been minimized.
This comment has been minimized.
|
cc @Amanieu, @folkertdev, @sayantn |
Having this patch up with the |
|
I don't think we can just remove it, this is in stable |
|
The feature is apparently stable, but none of the intrinsics are, so we can remove those at least I think |
| // FEAT_TME | ||
| ("tme", Stable, &[]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yeah removing a stable target feature will require T-lang involvement, even though I assume it'll be fine to remove the feature.
|
This causes build failures when using the latest LLVM when building tests (and so if we leave it in, anyone using the feature will hit a build failure on LLVM as soon as we update LLVM). If we want to keep |
|
we can just add a |
|
you can even make that conditional on the LLVM version, to not break anything overnight. I think that is the best step forward right now, and then T-lang can decide what to do long term. We can probably do a crater run and then remove it, but maybe they want to see a warning for a while. |
ARM has withdrawn FEAT_TME https://developer.arm.com/documentation/102105/lb-05/ LLVM has dropped support for it recently as a result.
While I understand keeping the feature around in case someone explicitly turned it on even though it does nothing, my understanding is that this feature was withdrawn because there are very few (no?) CPU implementations actually supporting it, just simulators and specs, so just ceasing to pass the feature shouldn't break anything. If you'd still prefer to, I'll re-alter the patch again, but I suppose there's no rush because we need to land the stdarch change first and merge it back. |
I understand, it's just that we have procedures to never merge any breaking changes to the language without the green light from T-lang. Things have slipped through in the past, and it's just easier to have that strict rule. I suspect T-lang will just rule to remove the feature, given that it likely has zero usage: all intrinsics to use it were unstable, I don't think this feature changes codegen, so the only theoretical risk is if someone wrote some code using inline assembly that relies on the target feature being enabled? |
ARM has withdrawn FEAT_TME
https://developer.arm.com/documentation/102105/lb-05/
LLVM has dropped support for generating it
llvm/llvm-project#167687
@rustbot label llvm-main
r? @durin42