Partially stabilize LoongArch target features by heiher · Pull Request #135015 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Stabilization PR for the LoongArch target features. This PR stabilizes some of the target features tracked by #44839.
Specifically, this PR stabilizes the following target features:
- f
- d
- frecipe
- lasx
- lbt
- lsx
- lvz
Docs PR: rust-lang/reference#1707
r? @Amanieu
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
Relevant to the library team, which will review and decide on the PR/issue.
labels
rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.
cc @rust-lang/rust-analyzer
This comment has been minimized.
If "d" and "f" work anything like the RISC-V features of the same name, they should not be stabilized yet. We need to figure out #116344 and #131799 for LoongArch first.
Does any of the other features ever have any effect on calling conventions or ABI?
If "d" and "f" work anything like the RISC-V features of the same name, they should not be stabilized yet. We need to figure out #116344 and #131799 for LoongArch first.
Does any of the other features ever have any effect on calling conventions or ABI?
Thank you! Other than f and d, there are no other target features affecting the calling convention and ABI.
Stabilize LoongArch target features Partially stabilize LoongArch target features
Comment on lines 773 to 774
| ("relax", STABLE, &[]), |
|---|
| ("ual", STABLE, &[]), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relaxrefers to linker relaxation support and may not be appropriate to expose as a target feature.ualrefers to support for unaligned memory access, similar tostrict-alignon ARM targets. It's probably fine to stabilize as-is, but we may want to double-check with other targets if we want to have a consistent feature name between all of them (alternatively it's also fine to make the feature target-specific).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also to be clear, ual can only affect how unaligned accesses are codegen'd, and nothing else. See this recent discussion on Zulip.
If we have target feature docs somewhere, they should clarify hat unaligned accesses are still UB except when performed via {read,write}_unaligned.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with skipping these two target features and stabilizing them at a more appropriate time.
Relevant to the language team, which will review and decide on the PR/issue.
label
@rfcbot concern what-about-d-and-f
In #135015 (comment), @RalfJung raised a concern:
If "d" and "f" work anything like the RISC-V features of the same name, they should not be stabilized yet. We need to figure out #116344 and #131799 for LoongArch first.
Does any of the other features ever have any effect on calling conventions or ABI?
Let's figure out the answer to that.
Ah I think the point is, there are now ABI checks set up for this, so the target features should be safe.
I hope they are, I don't have any experience with this target. The target is extremely similar to RISC-V though, and RISC-V has a lot more eyes on it and more people have experience in it, so it may make sense to stabilize "f" and "d" there first if we are confident about this. @Amanieu do you know if there are any blockers for that?
Also, @heiher given that 4 of the features on the original list have subtle ABI questions, could you double checks that all the other features are harmless in that regard?
Items that are on lang's radar and will need eventual work or consideration.
and removed I-lang-nominated
Nominated for discussion during a lang team meeting.
Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination
labels
Marks issues that should be documented in the release notes of the next release.
label
Also, @heiher given that 4 of the features on the original list have subtle ABI questions, could you double checks that all the other features are harmless in that regard?
f and d already have ABI compatibility checks in place. For lsx, 128-bit vectors are always passed through general-purpose registers, while for lasx, 256-bit vectors are always passed on the stack — so all four features are ABI-safe. The other features don’t affect the ABI and are harmless in that regard. Thanks.
Has the Final Comment Period ended? If so, I request to merge it. Thanks!
No, the bot will post when FCP has ended. So far, FCP has not even started.
Currently FCP is blocked on the concern registered by @traviscross here. I think that concern can be resolved since indeed the ABI check does account for "d" and "f" properly. I am a bit nervous since this is the first target feature where the ABI check becomes load-bearing on stable, but I guess that had to happen eventually. ;) It might be worth to double-check that we have good test coverage here. Can you edit tests/ui/target-feature/forbidden-hardfloat-target-feature-attribute.rs to use revisions: and also add a loongarch revision? (I know I was opposed to adding more tests in the past, but now that this moves to stable I am changing my mind. Also, adding a new revision as opposed to an entirely new test makes this much easier to maintain.) And we also need a test for the case of disabling a feature that should not be disabled; that could probably be a revision of tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable.rs.
EDIT: I am adding tests in #140395.
If you're happy then I'm happy, and we can...
@rfcbot resolve what-about-d-and-f
🔔 This is now entering its final comment period, as per the review above. 🔔
@heiher are all the features being stabilized here supported by our minimal LLVM version (which is v19 I think)? We should probably not stabilize target features that some rustc builds cannot support.
Zalathar added a commit to Zalathar/rust that referenced this pull request
…etrochenkov
organize and extend forbidden target feature tests
In particular this adds some loongarch tests for rust-lang#135015, Cc @heiher
Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.
Zalathar added a commit to Zalathar/rust that referenced this pull request
…etrochenkov
organize and extend forbidden target feature tests
In particular this adds some loongarch tests for rust-lang#135015, Cc @heiher
Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.
Zalathar added a commit to Zalathar/rust that referenced this pull request
…etrochenkov
organize and extend forbidden target feature tests
In particular this adds some loongarch tests for rust-lang#135015, Cc @heiher
Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.
bors added a commit to rust-lang-ci/rust that referenced this pull request
organize and extend forbidden target feature tests
In particular this adds some loongarch tests for rust-lang#135015, Cc @heiher
Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.
try-job: x86_64-gnu-llvm-*
bors added a commit to rust-lang-ci/rust that referenced this pull request
organize and extend forbidden target feature tests
In particular this adds some loongarch tests for rust-lang#135015, Cc @heiher
Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.
try-job: x86_64-gnu-llvm-20-*
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…etrochenkov
organize and extend forbidden target feature tests
In particular this adds some loongarch tests for rust-lang#135015, Cc @heiher
Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.
try-job: x86_64-gnu-llvm-20-*
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#140395 - RalfJung:target-feature-tests, r=petrochenkov
organize and extend forbidden target feature tests
In particular this adds some loongarch tests for rust-lang#135015, Cc @heiher
Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.
try-job: x86_64-gnu-llvm-20-*
Labels
Area: Enabling/disabling target features like AVX, Neon, etc.
This issue / PR is in PFCP or FCP with a disposition to merge it.
In the final comment period and will be merged soon unless new substantive objections are raised.
Items that are on lang's radar and will need eventual work or consideration.
Target: LoongArch (LA32R, LA32S, LA64)
Marks issues that should be documented in the release notes of the next release.
Status: Awaiting review from the assignee but also interested parties.
Relevant to the language team, which will review and decide on the PR/issue.