Skip to content

Need comprehensive story for target_feature compat #140570

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
workingjubilee opened this issue May 2, 2025 · 5 comments
Open

Need comprehensive story for target_feature compat #140570

workingjubilee opened this issue May 2, 2025 · 5 comments
Labels
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-bug Category: This is a bug. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@workingjubilee
Copy link
Member

From #138872 per Amanieu:

I do think we need a more general mechanism to indicate that 2 target features are mutually incompatible. For example:

  • RISC-V: Zfinx & F
  • RISC-V: I & E
  • ARM: mclass & rclass & aclass

Currently nothing stops you from enabling incompatible features with #[target_feature] and -C target-feature, which will crash LLVM.

We should be emitting an error way before anything gets to LLVM because we have to have a better idea of what features are actually compatible. We effectively need to have both global and local notions of it.

@workingjubilee workingjubilee added A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-bug Category: This is a bug. labels May 2, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 2, 2025
@workingjubilee
Copy link
Member Author

Most concerningly, relying on "let LLVM take us down if we do something foolish" doesn't always work and sometimes LLVM will happily emit horrifically invalid code.

@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 2, 2025
@a4lg
Copy link
Contributor

a4lg commented Jun 1, 2025

I'll summarize some of the RISC-V requirements:

Must be Implemented

Conflict Checking

For instance, the F extension ("f") and the Zfinx extension ("zfinx") are in conflict because they share the same encoding space, operates pretty much the same but the Zfinx extension operates on general purpose registers x0x31 (while the F extension operates on the separate set of 32 registers f0f31).

Also see the section "Custom Requirement Checking" below.

Should be Implemented

Extension Group

For instance, the A extension consists of two subsets: the Zalrsc and Zaamo extensions (A == Zalrsc + Zaamo).
Currently (in Rust), the A extension ("a") correctly implies "zalrsc" and "zaamo" but not vice versa (i.e. having both "zalrsc" and "zaamo" does not imply "a").

Reverse implication of an extension group is not described in RISC-V specifications but feature-based extension handling (i.e. if we have a feature, we have an extension; even if the specification does not specify extension dependencies) is more consistent and should be used on the language-end such as Rust and even LLVM (not Clang side but lower-end LLVM side) implements this (on the other hand, GNU Binutils largely follows the specifications, not implementing the extension groups).

That's why I did implement extension groups in the runtime feature detection logic for RISC-V (rust-lang/stdarch#1770) but didn't include documentation about extension groups in rust-lang/stdarch#1779 (section: Note: About Reverse Implication (Extension Groups); reverted by rust-lang/stdarch#1792 and partially restored in rust-lang/stdarch#1797) because documenting behavior of extension groups (currently not implemented in Rust's feature handling system via -Ctarget-feature= etc.) would confuse the users.

One of the ways to implement this is to implement Multiple Requirements I'll describe below.

Multiple Requirements

Some RISC-V extensions require multiple extensions to be implied (while current Rust's feature handling system only implements implication from a target feature). Currently, following two extensions require such handling and I excluded them from RISC-V feature addition PRs (as I explained in #139440):

  1. Zcd: implied from C ("c") and D ("d").
  2. Zcf: implied from target_arch = "riscv32", C ("c") and F ("f").

Reverse implication of an extension group can be implemented like this:

  • A ("a"): implied from Zalrsc ("zalrsc") and Zaamo ("zaamo").

Optional but Recommended

LLVM Codegen (at least): Additional Implication on Rust side after LLVM side Querying

In the LLVM Codegen, it first enables (default) target features by querying enabled features in LLVM (that itself is okay) but does not run implication logic after querying. If LLVM does not keep track of all relations of supported extensions, Rust will fail to enable specific target features (sorry, I strongly remember that I encountered a problem related to this but forgot which combination caused target_feature failures).

To deal with this, I recommend running implication after querying LLVM-enabled target features.

The same would apply to other codegen backends.

Optional

Custom Requirement Checking

In GNU Binutils, Zvl* extensions (denoting minimum vector register length) require at least one of the vector extension subsets (either full-set V or Zve* subsets).
This is because Zvl* extensions cannot imply any of vector extension subsets due to their nature (only determine minimum vector register length and not operations we are capable of).

I think this is not that important on the Rust side but if we prefer, implementing target feature checking (as well as conflict checking)

  • not as a fixed logic referring some kind of tables (describing e.g. F is incompatible to Zfinx)
  • but as a custom callback function (called after all target feature sets are converged)

would be nice.

@RalfJung
Copy link
Member

RalfJung commented Jun 1, 2025

operates pretty much the same but the Zfinx extension operates on GPRs (while the F extension operates on the separate set of 32 registers ― FPRs).

Please explain jargon-y acronyms before using them. :)
I assume GPR = general-purpose register, FPR = floating-point register?

@a4lg
Copy link
Contributor

a4lg commented Jun 1, 2025

@RalfJung Sorry, that's correct. I fixed the post above.

@a4lg
Copy link
Contributor

a4lg commented Jun 2, 2025

Supplement: Feature-based Extension Handling

Although that this is not completely irrelevant to the main subject, this is not important (here) compared to the post above, I note about that as the separate supplemental post.

This concept (the words I used above) means, handling existence of an extension strictly as a set of features implemented.

RISC-V defines numerous extensions and some of them are

  1. A superset of another (subset)
  2. but the extension does not define such subset as a dependency.

I'll provide an example:

The Zvknha and Zvknhb extensions implement accelerator instructions for the SHA-2 hashing functions (32-bit integer based SHA-256, 64-bit integer based SHA-512, and their derivatives like SHA-256→SHA-224 and SHA-512→SHA-512/224 (both shorter 224-bit hashing functions but computation part in the middle can be shared with corresponding bases)).

  • Zvknha: supports SHA-256
  • Zvknhb: supports SHA-256 and SHA-512

They completely share the instructions (the only difference is vector configuration prior to shared SHA-2 processing instructions: SHA-256 if EEW1=SEW2=32 and SHA-512 if EEW=SEW=64).

That means, the Zvknhb extension (SHA-256 and SHA-512) is functionally a strict superset of the Zvknha extension (SHA-256 only); i.e. all implementations with the Zvknhb extension effectively has the Zvknha extension.
However, the Zvknhb extension does not have the Zvknha extension as a dependency (nor implies it) on the documentation.

Currently, both LLVM and GNU toolchains chose not to imply the Zvknha extension from the Zvknhb extension.

However, on the language end (at least), this is inconvenient (would require like cfg(any(target_feature = "zvknha", target_feature = "zvknhb")) to configure by RISC-V vector support of SHA-256). That's why I implemented #140139 (which contains 4 instances of implication based on the feature-based extension handling including this).

Note:
I repeatedly push this handling even on the toolchains (such as GNU Binutils) but this handling itself is not widely accepted in the RISC-V toolchain ecosystem except the concept of extension groups as in LLVM.
Still, it slightly moved RISC-V spec authors to explicitly specify depending extensions/features ...and more importantly, complex implications related to the C extension is the result of the discussion I started based on this concept.

With that PR, the "zvknhb" target feature implies "zvknha" knowing that this is not on the specification (so I added a comment: Zvknhb ⊃ Zvknha) because all implementations with the Zvknhb extension effectively has the Zvknha extension.
So, to configure by RISC-V vector support of SHA-256, cfg(target_feature = "zvknha") is sufficient.

Footnotes

  1. EEW (Effective Element Width): Number of bits in a (scalar) element handled in the specific vector operation. On many vector operations (including all vector cryptography operations), it matches to SEW2.

  2. SEW (Selected Element Width): Number of bits selected as the one in an element of a vector and a part of vector state in the processor. This is set by vector configuration-setting instructions like vsetvli and vsetivli. 2

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. C-bug Category: This is a bug. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants