std_detect: RISC-V: Best effort implication of target features by custom cfg
#145810
Conversation
rustbot
added
S-waiting-on-author
RV32E specifically implies an RV32E environment and RV32E calling conventions. The
This seems fine. |
|
@Amanieu Thanks! I'll make a few adjustments and make this PR to leave the draft state. |
Internal calls to `$crate::detect_feature!` and `check_cfg_feature!` are long enough and we are going to add another option. This commit formats internal macro invocation for the future. It also reformats the contents of `check_cfg_feature!` to comply with `./x fmt check` (raises an error after modification of subsequent commits).
This commit adds an option to `@FEATURE` specification of the `feature!` macro: `implied by cfg(CUSTOM_CFG);`. It enables implying a target feature by custom configuration such like `all(target_feature = "A", target_feature = "B")`, which was impossible before this commit. The main reason adding this option is to properly support RISC-V "group" extensions (e.g. "A", which consists of "Zalrsc" and "Zaamo" subextensions) and other extensions with complex implications (like "Zcd", can be implied if both "C" and "D" are enabled but not vice versa). With this commit, we can make `is_riscv_feature_detected!` macro to properly detect existence of certain target features supplied via compiler options (sometimes exactly, on other times in a best-effort manner).
RISC-V has a sort of "group" extensions. For example, the "A" extension
consists of two subextensions "Zalrsc" and "Zaamo" and having two
subextensions is equivalent to having the "A" extension (group).
This commit implements implication of such target features exactly.
For "group" extensions (except "C"), implication logic looks like:
```
cfg(any(
target_feature = "group",
all(
target_feature = "feat1",
target_feature = "feat2",
target_feature = "feat3",
// ...
)
))
```
...where each `featN` is a non-group subextension and supporting
all `featN` is equivalent to the support of the entire group.
Testing `target_feature = "group"` is technically redundant on user programs
but kept here to test `cfg`s while compiling the standard library and
evaluation cost at compile-time should not be large.
On the RISC-V architecture, there are some complex implications around the "C" extension. This commit implements implication of such target features in a best effort manner. The reason why we cannot make this exact is lack of `"zcd"` and `"zcf"` target features (that would require additional feature handling system). Still, weaker implications implemented here are formally verified to be optimal if we have to avoid tautology (except `"c"`) and uses of `"zcf"` and `"zcd"` are prohibited. For "C", the author deduced minimal expression which makes equal to the "C" extension and extracted all cases where `"zcd"` and `"zcf"` are not involved. For "Zcd" and "Zcf", the author created proof statements, each stating that implication expression (which avoids tautology and accidentally avoids using `"zcd"` or `"zcf"`) is optimal to force corresponding target feature to be enabled.
`implied by target_features: [...]` is currently only used on the AArch64 architecture. Since those uses can be substituted with `implied by cfg(any(target_feature = ...))` or more simply `implied by cfg(target_feature = ...)` (if there's only 1 target feature to imply from), this commit attempts to use this for simplicity.
This commit removes `implied by target_features: [...];` inside the target description macro `features!` (which is superseded by the `implied by cfg(...);` syntax).
a4lg
force-pushed
the
std-detect-complex-implication
branch
from
a05d4bd to
99d68f3
Compare
rustbot
added
S-waiting-on-review
| /// "Zvkn" Cryptography Extension for NIST Algorithm Suite | ||
| @FEATURE: #[unstable(feature = "stdarch_riscv_feature_detection", issue = "111192")] zvknc: "zvknc"; | ||
| implied by cfg(any( | ||
| target_feature = "zvknc", |
There was a problem hiding this comment.
This line seems redundant and could be folded into the macro itself: a feature is always implied by itself.
There was a problem hiding this comment.
... unless implied by is used (even implied by target_features:, which is to be removed in this PR suppresses self implication; see the macro expansion carefully).
So, zvknc runtime (std_detect) target feature is not directly implied by zvknc compiler target feature itself.
Still, you are technically right since... for instance (in this place), zvknc is just a collection of sub-extensions (zvkned, zvknhb...) and zvknc compiler target feature directly/indirectly enables all of them (indirectly enabling runtime target feature zvknc).
The reason I keep zvknc itself is to check validity of the compiler target feature (through check_cfg_feature!).
I think we can keep this (and similar lines) as-is (in this PR).
|
I've been thinking about this PR a bit and I think that the best place for all the feature implication logic is actually in rustc itself rather than here. This will ensure that feature checking with |
3 participants
This PR implements new feature to
std_detect'sfeature!macro: implication of target features by customcfgand then uses this to implement static implication logic on RISC-V where the standard target feature handling does not capture all the cases.This PR implements implication logic for three kinds of target features:
For detailed intent of the changes, see each commit message.
It also changes some AArch64 code to use the new syntax added in this PR and removes the old custom syntax to imply a target feature.
cf. Z3-based prover for complex implications around the "C" extension
Draft Design Consideration 1: RISC-V: How to handle ISA Bases?
Implementing custom implications to base ISA target features like
rv32imight be an option but I have a concern (which will not happen for now but plausible in the future).In particular, I have a concern on the following case: the program is built for an
rv32etarget but ran on anrv32ienvironment (which is a superset ofrv32e) and reports existence ofrv32i. If that happens, bothrv32eandrv32iwould returntrueonstd::arch::is_riscv_feature_detected!.So, the commit implementing implication of base ISA target features (originally commit 3) was marked DRAFT.
My original position: Slightly Negative.
Conclusion: Remove original commit 3 and do not imply ISA bases.
Draft Design Consideration 2: Substituting
implied by target_features:This PR (originally commit 2) implements implication by custom
cfgwith following syntax:implied by cfg(CFG);.Unlike existing
implied by target_features: [A, B, ...];(which will work likecfg(any(target_feature = "A", target_feature = "B", ...))), this syntax accepts anycfgs.Interestingly,
implied by target_features:is currently only used on AArch64 and in the simplest form (implied from one target feature). So if there's no good reason keeping this, we have an option to removeimplied by target_features: [...];entirely and substitute withimplied by cfg(...);.Commits that make such changes (originally commits 6 and 7) were marked DRAFT.
My original position: Positive (but leaving draft after hearing from others).
Conclusion: Substitute as the original.
r? @Amanieu
@rustbot label +O-riscv