Stabilize Ratified RISC-V Target Features

[coastalwhite]

Stabilization PR for the ratified RISC-V target features. This stabilizes some of the target features tracked by #44839. This is also a part of #114544 and eventually needed for the RISC-V part of rust-lang/rfcs#3268.

There is a similar PR for the the stdarch crate which can be found at rust-lang/stdarch#1476.

This was briefly discussed on Zulip
(https://rust-lang.zulipchat.com/#narrow/stream/250483-t-compiler.2Frisc-v/topic/Stabilization.20of.20RISC-V.20Target.20Features/near/394793704).

Specifically, this PR stabilizes the:

• Atomic Instructions (A) on v2.0
• Compressed Instructions (C) on v2.0
• Double-Precision Floating-Point (D) on v2.2
• Embedded Base (E) (Given as RV32E / RV64E) on v2.0
• Single-Precision Floating-Point (F) on v2.2
• Integer Multiplication and Division (M) on v2.0
• Vector Operations (V) on v1.0
• Bit Manipulations (B) on v1.0 listed as zba, zbc, zbs
• Scalar Cryptography (Zk) v1.0.1 listed as zk, zkn, zknd, zkne, zknh, zkr, zks, zksed, zksh, zkt, zbkb, zbkc zkbx
• Double-Precision Floating-Point in Integer Register (Zdinx) on v1.0
• Half-Precision Floating-Point (Zfh) on v1.0
• Minimal Half-Precision Floating-Point (Zfhmin) on v1.0
• Single-Precision Floating-Point in Integer Register (Zfinx) on v1.0
• Half-Precision Floating-Point in Integer Register (Zhinx) on v1.0
• Minimal Half-Precision Floating-Point in Integer Register (Zhinxmin) on v1.0

r? @Amanieu

[Amanieu]

This is an FCP for both adding the target features to the language via #[target_feature] and for adding them to the standard library via is_riscv_feature_detected!. These features (including the feature names) have been standardized by the RISC-V foundation.

@rfcbot fcp merge

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

No concerns currently listed.

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.

[RalfJung]

On x86 we are having some trouble because it is possible to disable features that are enabled by default.

Are there any such RISC-V features? Can we ensure that all features that our targets have by default cannot be disabled by the user?

[Amanieu]

On RISC-V the ABI is explicitly specified in the target spec, independently of which target features are used. Obviously, you need to enable the FP instructions if you are using a hard-float ABI, but otherwise I don't think this would cause any issues.

[RalfJung]

What happens if you write a function with float argument/return types and you have the FP instruction feature disabled? How do arguments get passed? Or does codegen abort in that case?

[Amanieu]

I think LLVM just fails with an assert.

[RalfJung]

We should really ensure this is the case, with a test. Fixing "target features are affecting the ABI" later is such a pain.

[coastalwhite]

I am happy to write a test for this. This will do this on Monday.

[ehuss]

Can you please post a PR to update the documentation at https://github.com/rust-lang/reference/blob/HEAD/src/attributes/codegen.md#the-target_feature-attribute?

[coastalwhite]

Can you please post a PR to update the documentation at https://github.com/rust-lang/reference/blob/HEAD/src/attributes/codegen.md#the-target_feature-attribute?

I will do that as well. I noticed that there is a patch necessary as well on the page for in-line assembly. Vector registers are supported as register in the inline assembly already.

[Amanieu]

I will do that as well. I noticed that there is a patch necessary as well on the page for in-line assembly. Vector registers are supported as register in the inline assembly already.

There are currently no valid types that you can pass as input/output asm operands for vector registers. This will be solved once we have support for scalable vectors in stdarch.

[coastalwhite]

Passing in("v3") worked for before, but maybe Rust doesn't officially support that.

[Amanieu]

Passing in("v3") worked for before, but maybe Rust doesn't officially support that.

I don't think it ever worked:

error: register class `vreg` can only be used as a clobber, not as an input or output
 --> <source>:4:10
  |
4 | asm!("", in("v3") x);
  |          ^^^^^^^^^^

error: type `i32` cannot be used with this register class
 --> <source>:4:19
  |
4 | asm!("", in("v3") x);
  |                   ^
  |
  = note: register class `vreg` supports these types: 

error: aborting due to 2 previous errors

[coastalwhite]

This (https://godbolt.org/z/rcGTfbMM7) works for me, but it is on an out("v0").

[Amanieu]

Yes, you can only use it as a clobber with _. You can't actually move values in/out of the asm.

[coastalwhite]

Ah, I could have sworn that the Only clobbers in the reference was not there before, I must have overlooked it. Never mind then.

[coastalwhite]

We should really ensure this is the case, with a test. Fixing "target features are affecting the ABI" later is such a pain.

It seems like this currently does not fail codegen. We get messages saying what we are doing is probably incorrect, but no exit on codegen. It actually just ignores the target ABI and falls back to the ABI without hard floats.

The message:

Hard-float 'd' ABI can't be used for a target that doesn't support the D instruction set extension (ignoring target-abi)

Here is the reproduction on Compiler Explorer as well. It shows the same message and also does not fail.

[RalfJung]

So, it's then generating a function that is not ABI-compatible with a fn(f64) -> f64 function that was generated with hard floats available? Yeah that's exactly the same as #116344, let's not stabilize another instance of this mess please.

It's good to get a warning at least. I wonder where the warning comes from, it doesn't look like a rustc diagnostic...

[coastalwhite]

@RalfJung Is there something that Rust can do to fix this internally, or is this a LLVM issue?

[RalfJung]

For x86 the tentative plan was to reject -x87 as a feature flag, thus ensuring we won't generate functions with the wrong ABI. An alternative we are considering in #116558 is to reject certain types to be passed by-value when the corresponding ABI features are missing.

What's not quite clear is how to handle softfloat situations. Presumably if you are using softfloats you want to be able to pass them around by-value. IMO we should have this distinction on the target level: i686-unknown-linux-gnu is a hardfloat target and -87 is banned, so floats are always passed in appropriate SSE/x87 registers; x86_64-unknown-none is a softfloat target and x87 (and all other float/vector-related features) are banned so floats and SIMD types are always passed on the stack or in general-purpose registers. With some work on the LLVM side, ideally we could allow the target features without affecting ABI. The important part is that for any given target triple, if compilation succeeds, then all extern "C" functions that take or return f32 or __m256 always do that the same way no matter the available target features.

(I'm using x86 terms here since that's what I am more familiar with, I assume these concepts all exist for RISC-V as well with different names. For instance it seems like d,f is the equivalent to x87. But the questions and possible solutions should all carry over and in rustc this should all be using the same infrastructure, but so far none of that infrastructure exists.)

[RalfJung]

Looking at the feature list, f, d, and v seem to have the highest risk of affecting ABI. I played around with this a bit and this is as far as I got -- there's a clear difference visible for d affecting f64 ABI, but I at least for that function it looks like v doesn't affect the ABI of f32x4? Obviously when I make it actually do something it uses different instructions, but I cannot tell if the input/output is passed via the same registers.

[coastalwhite]

RISC-V does not have traditional fixed size SIMD instructions, it instead uses scalable vectors. Portable SIMD for RISC-V afaik calls the p extension (when available) which has not been ratified yet. I would not want the p extension to be part of this stabilization PR. The v target feature currently only allows for implicit codegen (or inline assembly). Explicit codegen with intrinsics is tracked in rust-lang/rfcs#3268.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[traviscross]

@rustbot labels -I-lang-nominated

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[Amanieu]

@bors r+

[bors]

📌 Commit 6988d22 has been approved by Amanieu

It is now in the queue for this repository.

[bors]

⌛ Testing commit 6988d22 with merge 91bbdd9...

[bors]

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 91bbdd9 to master...

[rust-timer]

Finished benchmarking commit (91bbdd9): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.3%	[4.0%, 4.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 635.247s -> 634.756s (-0.08%)
Artifact size: 304.45 MiB -> 304.45 MiB (0.00%)

[vishalverma8200]

---

[kupiakos]

#116485 does not mention the zbb extension but the diff is the same as for zba, zbc, and zbs. Should it be edited?

[coastalwhite]

Indeed zbb is also stabilised, what do you think should it be edited?