Skip to content
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

rustc_target: Add RISC-V atomic-related features #130877

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Sep 26, 2024

This adds the following three target features to unstable riscv_target_feature.

(Zacas Extension is not included here because it is still marked as experimental in LLVM 19 llvm/llvm-project@70e7d26 and will become non-experimental in LLVM 20 llvm/llvm-project@614aeda)

a implies zaamo and zalrsc, and zabha implies zaamo:

r? @Amanieu

@rustbot label +O-riscv +A-target-feature

@rustbot
Copy link
Collaborator

rustbot commented Sep 26, 2024

Some changes occurred in tests/ui/check-cfg

cc @Urgau

@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. A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. O-riscv Target: RISC-V architecture labels Sep 26, 2024
@michaelmaitland
Copy link

I didn't make a implies zaamo and zalrsc, or zabha implies zaamo for now:

According to d9715c698c4940e65e9bd036e22fb5cab3aa2c6f,

Zabha and Zacas are both documented as depending on Zaamo. I'm
hesitant to make them imply Zaamo instead.

So remove the implication and replace with a check that either
A or Zaamo is enabled.

I think that we still need a check like this?

@taiki-e
Copy link
Member Author

taiki-e commented Sep 26, 2024

Zabha and Zacas are both documented as depending on Zaamo. I'm
hesitant to make them imply Zaamo instead.
So remove the implication and replace with a check that either
A or Zaamo is enabled.

I think that we still need a check like this?

Thanks for pointing this out. If we go in the same direction as LLVM, we certainly need to do that.
Currently, the LLVM backend will cause an LLVM error (https://godbolt.org/z/a8vb8GxYc), but other backends will probably not.

That said, I'm beginning to think that it is not actually necessary to separate what the ISA states ".. depends upon ..".

@taiki-e
Copy link
Member Author

taiki-e commented Sep 27, 2024

Updated to make a implies zaamo and zalrsc, and zabha implies zaamo.

@michaelmaitland
Copy link

I got some more context on why LLVM is the way it is.

The RISC-V ISA spec is a hardware spec not a toolchain spec, so we must understand this context.

The A extension was later split into Zaamo and Zalrsc. When hardware implements Zaamo and Zalrsc, it implemented A. End of story on the hardware side.

But on the toolchain side, things are a little messy, especially when we are dealing with mixed versions of software in the toolchain. If a is made to imply Zaamo or Zalrsc in the compiler, and we used an older but stable version of binutils that was shipped before a was split, then that stable binutils becomes unusable with the newer compiler, even though the a spec has not changed.

This is the reason why LLVM does not have the implication. GCC + binutils usually ship with the same version, so this incompatibility is less of a worry. This is why the GCC folks were willing to add the implication here. LLVM decided that it would enforce that if you're going to use zacas or zabha, then you better enable a or zaamo, which ensures that we have what is needed without breaking what I talked about above.

I don't want to be the one to say how it should be in rust, but I figured that this context could be valuable for those who are making the decision.

@bors
Copy link
Contributor

bors commented Sep 27, 2024

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

@Amanieu
Copy link
Member

Amanieu commented Sep 30, 2024

My understanding is that this may cause issues with an old binutils, but I believe newer binutils only warn instead of error when encountering unknown features. In any case, we can revert this and re-evaluate if it causes problems in nightly.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 30, 2024

📌 Commit 62612af has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 30, 2024
Rollup of 5 pull requests

Successful merges:

 - rust-lang#129638 (Hook up std::net to wasi-libc on wasm32-wasip2 target)
 - rust-lang#130877 (rustc_target: Add RISC-V atomic-related features)
 - rust-lang#130914 (Mark some more types as having insignificant dtor)
 - rust-lang#130961 (Enable `f16` tests on x86 Apple platforms)
 - rust-lang#130966 (make ptr metadata functions callable from stable const fn)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit acaa6ce into rust-lang:master Oct 1, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 1, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 1, 2024
Rollup merge of rust-lang#130877 - taiki-e:riscv-atomic, r=Amanieu

rustc_target: Add RISC-V atomic-related features

This adds the following three target features to unstable riscv_target_feature.

- `zaamo` (Zaamo Extension 1.0.0): Atomic Memory Operations (`amo*.{w,d}{,.aq,.rl,.aqrl}`)
  ([definition in LLVM](https://github.com/llvm/llvm-project/blob/llvmorg-19.1.0/llvm/lib/Target/RISCV/RISCVFeatures.td#L229-L231), [available since LLVM 19](llvm/llvm-project@8be079c))
- `zabha` (Zabha Extension 1.0.0): Byte and Halfword Atomic Memory Operations (`amo*.{b,h}{,.aq,.rl,.aqrl}`)
  ([definition in LLVM](https://github.com/llvm/llvm-project/blob/llvmorg-19.1.0/llvm/lib/Target/RISCV/RISCVFeatures.td#L238-L240), [available since LLVM 19](llvm/llvm-project@6b74449))
- `zalrsc` (Zalrsc Extension 1.0.0): Load-Reserved/Store-Conditional Instructions (`lr.{w,d}{,.aq,.rl,.aqrl}` and `sc.{w,d}{,.aq,.rl,.aqrl}`)
  ([definition in LLVM](https://github.com/llvm/llvm-project/blob/llvmorg-19.1.0/llvm/lib/Target/RISCV/RISCVFeatures.td#L261-L263), [available since LLVM 19](llvm/llvm-project@8be079c))

(Zacas Extension is not included here because it is still marked as experimental in LLVM 19 llvm/llvm-project@70e7d26 and will become non-experimental in LLVM 20 llvm/llvm-project@614aeda)

`a` implies `zaamo` and `zalrsc`, and `zabha` implies `zaamo`:

- After Zaamo and Zalrsc Extensions are frozen, riscv-isa-manual says "The A extension comprises instructions provided by the Zaamo and Zalrsc extensions" (riscv/riscv-isa-manual@e87412e), and [`a` implies `zaamo` and `zalrsc` in GCC](https://github.com/gcc-mirror/gcc/blob/08693e29ec186fd7941d0b73d4d466388971fe2f/gcc/config/riscv/arch-canonicalize#L44). However, in LLVM, [`a` does not define them as implying `zaamo` and `zalrsc`](https://github.com/llvm/llvm-project/blob/llvmorg-19.1.0/llvm/lib/Target/RISCV/RISCVFeatures.td#L206).
- Zabha and Zaamo are in a similar situation, [riscv-isa-manual](https://github.com/riscv/riscv-isa-manual/blob/main/src/zabha.adoc) says "The Zabha extension depends upon the Zaamo standard extension", and [`zabha` implies `zaamo` in GCC](https://github.com/gcc-mirror/gcc/blob/08693e29ec186fd7941d0b73d4d466388971fe2f/gcc/config/riscv/arch-canonicalize#L45-L46), but [does not in LLVM (but enabling `zabha` without `zaamo` or `a` is not allowed)](https://github.com/llvm/llvm-project/blob/llvmorg-19.1.0/llvm/lib/TargetParser/RISCVISAInfo.cpp#L776-L778).

r? `@Amanieu`

`@rustbot` label +O-riscv +A-target-feature
@taiki-e taiki-e deleted the riscv-atomic branch October 1, 2024 01:44
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. O-riscv Target: RISC-V architecture S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants