Skip to content

Partially stabilize LoongArch target features #135015

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

heiher
Copy link
Contributor

@heiher heiher commented Jan 2, 2025

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 2, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2025

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

@heiher heiher force-pushed the stabilize-loongarch-target-features branch from cb6584c to 29c94f5 Compare January 2, 2025 07:35
@jieyouxu jieyouxu added the O-loongarch Target: LoongArch (LA32R, LA32S, LA64) label Jan 2, 2025
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Jan 2, 2025

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?

@jieyouxu jieyouxu added the A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. label Jan 2, 2025
@heiher
Copy link
Contributor Author

heiher commented Jan 2, 2025

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.

@heiher heiher changed the title Stabilize LoongArch target features Partially stabilize LoongArch target features Jan 2, 2025
@heiher heiher force-pushed the stabilize-loongarch-target-features branch from 29c94f5 to 4c59f0d Compare January 2, 2025 14:16
@ehuss
Copy link
Contributor

ehuss commented Jan 2, 2025

This will need a PR for documentation at https://github.com/rust-lang/reference/blob/master/src/attributes/codegen.md, and a lang (+whoever?) FCP.

Comment on lines 773 to 774
("relax", STABLE, &[]),
("ual", STABLE, &[]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • relax refers to linker relaxation support and may not be appropriate to expose as a target feature.
  • ual refers to support for unaligned memory access, similar to strict-align on 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).

Copy link
Member

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.

Copy link
Contributor Author

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.

@Amanieu
Copy link
Member

Amanieu commented Jan 2, 2025

r? lang

@rustbot rustbot added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jan 2, 2025
@rustbot rustbot assigned joshtriplett and unassigned Amanieu Jan 2, 2025
@heiher
Copy link
Contributor Author

heiher commented Jan 3, 2025

This will need a PR for documentation at https://github.com/rust-lang/reference/blob/master/src/attributes/codegen.md, and a lang (+whoever?) FCP.

PR: rust-lang/reference#1707

@heiher heiher force-pushed the stabilize-loongarch-target-features branch from 4c59f0d to 71e17a0 Compare January 3, 2025 07:10
@bors
Copy link
Collaborator

bors commented Jan 6, 2025

☔ The latest upstream changes (presumably #134794) made this pull request unmergeable. Please resolve the merge conflicts.

@heiher heiher force-pushed the stabilize-loongarch-target-features branch from 71e17a0 to 49b6947 Compare January 6, 2025 06:56
@traviscross traviscross added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 7, 2025
@bors
Copy link
Collaborator

bors commented Mar 15, 2025

☔ The latest upstream changes (presumably #138523) made this pull request unmergeable. Please resolve the merge conflicts.

@heiher heiher force-pushed the stabilize-loongarch-target-features branch from 49b6947 to f4c604d Compare March 18, 2025 09:48
@heiher heiher requested a review from Amanieu March 18, 2025 11:07
@heiher heiher force-pushed the stabilize-loongarch-target-features branch from f4c604d to 7cbc101 Compare March 19, 2025 07:20
@heiher heiher force-pushed the stabilize-loongarch-target-features branch from 7cbc101 to d5c5a4a Compare March 19, 2025 07:35
@rust-log-analyzer

This comment has been minimized.

@heiher heiher force-pushed the stabilize-loongarch-target-features branch from d5c5a4a to 1f2536f Compare March 19, 2025 08:10
@Amanieu Amanieu added the I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination label Mar 19, 2025
@heiher
Copy link
Contributor Author

heiher commented Mar 21, 2025

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.

That's resolved. Can we start the FCP now?

@heiher
Copy link
Contributor Author

heiher commented Apr 3, 2025

r? lang (A gentle ping. :)

@rustbot rustbot assigned scottmcm and unassigned joshtriplett Apr 3, 2025
@heiher
Copy link
Contributor Author

heiher commented Apr 10, 2025

I think this is ready for final comment period. Could someone from the lang team take a look?

cc @rust-lang/lang

@scottmcm scottmcm assigned joshtriplett and unassigned scottmcm Apr 16, 2025
@traviscross traviscross removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 16, 2025
@RalfJung
Copy link
Member

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.

That's resolved. Can we start the FCP now?

Has it? They are still listed in the PR description.

@traviscross
Copy link
Contributor

Amanieu joined us on the call today to talk us through this. This seems fine, modulo what @RalfJung had mentioned about "d" and "f", so I'm going to propose FCP merge and file an immediate concern to that effect.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 16, 2025

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 16, 2025
@tmandry
Copy link
Member

tmandry commented Apr 16, 2025

@rfcbot reviewed

@traviscross
Copy link
Contributor

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

@RalfJung
Copy link
Member

RalfJung commented Apr 16, 2025

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?

@RalfJung
Copy link
Member

RalfJung commented Apr 16, 2025

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?

@traviscross traviscross added I-lang-radar 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. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination labels Apr 16, 2025
@Amanieu Amanieu added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 16, 2025
@heiher
Copy link
Contributor Author

heiher commented Apr 16, 2025

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.

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. I-lang-radar Items that are on lang's radar and will need eventual work or consideration. O-loongarch Target: LoongArch (LA32R, LA32S, LA64) proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.