Remove useless function multiversioning features #315

andrewcarlotti · 2024-03-28T18:32:52Z

Supporting a feature in function multiversioning requires that the rest of the toolchain can support enabling the feature. In GCC/Binutils, this requires either that there is an equivalent feature extension available in the -march command line option, or that the feature introduces no new instructions (and therefore does not need to be enabled when passing to the assembler). However, many of the features listed in in the original function multiversioning specification do not meet this criteria. These fall into four categories:

Features that were originally linked to a specific architecture version (fcma, jscvt, frintts, flagm2, wfxt, rcpc2): these should get their own flags across the toolchain (and I've already added this support in Binutils).
Features that are combined with other features in existing command line options: these should also be merged in the function multiversioning specification.
Features that indicate support for hint instructions: these can be dropped, since the instructions can be used unconditionally.
Features that enable existing instruction behaviour to be changed when a system register flag is set: the function multiversioning resolvers don't check for runtime enablement of the control flags, so this isn't a suitable way of exploiting the behaviour enabled by those flags.
Features that can also be expressed as a combination of two other features.

We therefore remove support for the following features from the specification:

sha1 (2): included within +sha2
pmull (2): included within +aes
dit (4)
dgh (3)
ebf16 (4)
sve-bf16 (5)
sve-ebf16 (4)
sve-i8mm (5)
sve2-pmull128 (2): included within +sve2-aes
memtag2 (2): included within +memtag
memtag3 (4)
ssbs2 (2): included within +ssbs
bti (3)
ls64_v, ls64_accdata (2): included within +ls64

name: Pull request
about: Technical issues, document format problems, bugs in scripts or feature proposal.

Thank you for submitting a pull request!

If this PR is about a bugfix:

Please use the bugfix label and make sure to go through the checklist below.

If this PR is about a proposal:

We are looking forward to evaluate your proposal, and if possible to
make it part of the Arm C Language Extension (ACLE) specifications.

We would like to encourage you reading through the contribution
guidelines, in particular the section on submitting
a proposal.

Please use the proposal label.

As for any pull request, please make sure to go through the below
checklist.

Checklist: (mark with X those which apply)

If an issue reporting the bug exists, I have mentioned it in the
PR (do not bother creating the issue if all you want to do is
fixing the bug yourself).
I have added/updated the SPDX-FileCopyrightText lines on top
of any file I have edited. Format is SPDX-FileCopyrightText: Copyright {year} {entity or name} <{contact informations}>
(Please update existing copyright lines if applicable. You can
specify year ranges with hyphen , as in 2017-2019, and use
commas to separate gaps, as in 2018-2020, 2022).
I have updated the Copyright section of the sources of the
specification I have edited (this will show up in the text
rendered in the PDF and other output format supported). The
format is the same described in the previous item.
I have run the CI scripts (if applicable, as they might be
tricky to set up on non-*nix machines). The sequence can be
found in the contribution
guidelines. Don't
worry if you cannot run these scripts on your machine, your
patch will be automatically checked in the Actions of the pull
request.
I have added an item that describes the changes I have
introduced in this PR in the section Changes for next
release of the section Change Control/Document history
of the document. Create Changes for next release if it does
not exist. Notice that changes that are not modifying the
content and rendering of the specifications (both HTML and PDF)
do not need to be listed.
When modifying content and/or its rendering, I have checked the
correctness of the result in the PDF output (please refer to the
instructions on how to build the PDFs
locally).
The variable draftversion is set to true in the YAML header
of the sources of the specifications I have modified.
Please DO NOT add my GitHub profile to the list of contributors
in the README page of the project.

Supporting a feature in function multiversioning requires that the rest of the toolchain can support enabling the feature. In GCC/Binutils, this requires either that there is an equivalent feature extension available in the `-march` command line option, or that the feature introduces no new instructions (and therefore does not need to be enabled when passing to the assembler). However, many of the features listed in in the original function multiversioning specification do not meet this criteria. These fall into four categories: 1. Features that were originally linked to a specific architecture version (fcma, jscvt, frintts, flagm2, wfxt, rcpc2): these should get their own flags across the toolchain (and I've already added this support in Binutils). 2. Features that are combined with other features in existing command line options: these should also be merged in the function multiversioning specification. 3. Features that indicate support for hint instructions: these can be dropped, since the instructions can be used unconditionally. 4. Features that enable existing instruction behaviour to be changed when a system register flag is set: the function multiversioning resolvers don't check for runtime enablement of the control flags, so this isn't a suitable way of exploiting the behaviour enabled by those flags. 5. Features that can also be expressed as a combination of two other features. We therefore remove support for the following features from the specification: - sha1 (2): included within +sha2 - pmull (2): included within +aes - dit (4) - dgh (3) - ebf16 (4) - sve-bf16 (5) - sve-ebf16 (4) - sve-i8mm (5) - sve2-pmull128 (2): included within +sve2-aes - memtag2 (2): included within +memtag - memtag3 (4) - ssbs2 (2): included within +ssbs - bti (3) - ls64_v, ls64_accdata (2): included within +ls64

It currently isn't possible to support function multiversioning features properly in GCC without also enabling the extension in the command line options (with the exception of features such as "rpres" that do not require assembler support). We therefore remove unsupported features from GCC's list of FMV features. Some of these features ("fcma", "jscvt", "frintts", "flagm2", "wfxt", "rcpc2", and perhaps "dpb" and "dpb2") will be added back in the future once support for the command line option has been added. The rest of the removed features I have proposed removing from the ACLE specification as well, since it doesn't seem worthwhile to include support for them; see the ACLE pull request for more detailed justification: ARM-software/acle#315 gcc/ChangeLog: * config/aarch64/aarch64-option-extensions.def: Remove "flagm2", "sha1", "pmull", "dit", "dpb", "dpb2", "jscvt", "fcma", "rcpc2", "frintts", "dgh", "ebf16", "sve-bf16", "sve-ebf16", "sve-i8mm", "sve2-pmull128", "memtag3", "bti" and "wfxt" entries.

Some architecture features have been combined under a single command line flag, but have been assigned multiple FMV feature names with the command line flag name enabling only a subset of these features in the FMV specification. I've proposed reallocating names in the FMV specification to match the command line flags [1], but for GCC 14 we'll just remove them from the FMV feature list. [1] ARM-software/acle#315 gcc/ChangeLog: * config/aarch64/aarch64-option-extensions.def: Remove "memtag", "memtag2", "ssbs", "ssbs2", "ls64", "ls64_v" and "ls64_accdata" FMV features.

labrinea · 2024-04-15T11:53:32Z

All the suggested changes LGTM. Another feature in question that I believe falls in the second category is "ssbs" (included within +ssbs2) and one that falls in the forth category is "rpres".

labrinea · 2024-04-15T12:26:47Z

All the suggested changes LGTM. Another feature in question that I believe falls in the second category is "ssbs" (included within +ssbs2) and one that falls in the forth category is "rpres".

Oh I realised ssbs was already on your list, I must have missed it.

As explained in ARM-software/acle#315 we are deprecating features which aren't adding any value.

As explained in ARM-software/acle#315 we are deprecating features which aren't adding any value. These are: sha1, pmull, dit, dgh, ebf16, sve-bf16, sve-ebf16, sve-i8mm, sve2-pmull128, memtag2, memtag3, ssbs2, bti, ls64_v, ls64_accdata

This patch is sorting out inconsistencies in TargetParser regarding: * features without corresponding ArchExtKind * features without (Neg)Feature string * features with incorrect DependentFeatures string Also fixes "fp" which incorrectly implied "neon". This leaves us with "rpres" being the only remaining FMV feature with an incomplete entry. We are are currently reviewing it in ARM-software/acle#315. Having accurate ExtensionInfo entries in TargetParser is the only way of propagating the FMV information from clang to LLVM. If we don't want to rely on that we have to come up with another plan of encoding this information in LLVM-IR. For now we are relying on target-features. The PR llvm#87939 is an example of why we need this.

In ACLE we are deprecating a bunch of FMV features (see ARM-software/acle#315), so in this patch I am removing the corresponding tests.

As explained in ARM-software/acle#315 we are deprecating features which aren't adding any value. These are: sha1, pmull, dit, dgh, ebf16, sve-bf16, sve-ebf16, sve-i8mm, sve2-pmull128, memtag2, memtag3, ssbs2, bti, ls64_v, ls64_accdata

DanielKristofKiss · 2024-04-18T14:05:29Z

main/acle.md

   | 470           | `FEAT_SB`                | sb            | ```ID_AA64ISAR1_EL1.SB == 0b0001```       |
   | 480           | `FEAT_SPECRES`           | predres       | ```ID_AA64ISAR1_EL1.SPECRES == 0b0001```  |
-   | 490           | `FEAT_SSBS`              | ssbs          | ```ID_AA64PFR1_EL1.SSBS == 0b0001```      |
-   | 500           | `FEAT_SSBS2`             | ssbs2         | ```ID_AA64PFR1_EL1.SSBS == 0b0010```      |
-   | 510           | `FEAT_BTI`               | bti           | ```ID_AA64PFR1_EL1.BT == 0b0001```        |


Please don't drop BTI it is used to detect the feature and generate different code/functionality based on it.
see: https://chromium-review.googlesource.com/c/chromium/src/+/5453915

So, as I understand it, that use chromium example is using FMV on a function that converts a PageAccessibilityConfiguration into an integer mask that will be passed to mmap calls. The FMV usage is to determine whether the PROT_BTI and PROT_MTE bits should be set in the mask. The main motivation for switching to FMV in this patch is that caching the feature identification check in the GOT is supposedly more resistant to modification by an attacker global structure.

One thing that's not clear to me is whether the check is necessary at all. It looks like the Linux kernel might ignore just PROT_MTE when the system doesn't support it, so maybe it's safe to always pass PROT_MTE in the bitmask. Perhaps the same is true for PROT_BTI. However, I wouldn't fully trust my limited code reading, so might be mistaken.

It's possible to support BTI without adding new command line options, so I'm not particularly opposed to retaining this feature if it is actually useful. It originally felt like there ought to be a better way to handle this use case, but on further consideration approach seems reasonable (assuming that the kernel requires the PROT_BTI and PROT_MTE bits to be unset).

Ah - it does check:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/mman.h#n38
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/mprotect.c#n706

DanielKristofKiss · 2024-04-18T14:07:32Z

"Features that indicate support for hint instructions: these can be dropped, since the instructions can be used unconditionally."
but still the functionality may matters. FMV could be used for other stuff beside codegen.

andrewcarlotti · 2024-04-22T17:33:11Z

All the suggested changes LGTM. Another feature in question that I believe falls in the second category is "ssbs" (included within +ssbs2) and one that falls in the forth category is "rpres".

Ah - I hadn't noticed that FEAT_RPRES only applied when FEAT_AFP Alternate Handling is enabled. That probably put it in my fourth category as well, although it's slightly less clear cut than the other cases.

Wilco1 · 2024-06-05T12:59:29Z

Can we progress this? It seems there is agreement on most of these, so just keep FEAT_BTI.

labrinea · 2024-07-09T13:36:53Z

Ping @DanielKristofKiss @rsandifo-arm

DanielKristofKiss · 2024-07-09T13:43:37Z

@labrinea I think we need new version of this PR to reflect above comments and also has merge conflicts.

It was raised in ARM-software#315 that some features are of no use. In this revision I am removing FMV features that have no equivalent backend feature in the compiler therefore cannot be supported in any meaningful way [1], and I am fusing FMV features that the compiler cannot support independently [2]. [1]: dgh, rpres, sve-bf16, sve-ebf16, sve-i8mm, memtag3 [2]: {sha1, sha2}, {aes, pmull}, {sve2-aes, sve2-pmull128}, {memtag, memtag2}, {ssbs, ssbs2}, {ls64, ls64_v, ls64_accdata}

It was raised in ARM-software#315 that some features are of no use. In this revision I am removing FMV features that have no equivalent backend feature in the compiler therefore cannot be supported in any meaningful way [1], and I am fusing FMV features that the compiler cannot support independently [2]. [1]: dgh, ebf16, rpres, sve-bf16, sve-ebf16, sve-i8mm, memtag3 [2]: {sha1, sha2}, {aes, pmull}, {sve2-aes, sve2-pmull128}, {memtag, memtag2}, {ssbs, ssbs2}, {ls64, ls64_v, ls64_accdata}

vhscampos · 2024-08-06T08:35:21Z

Hi all, can you please confirm if this patch has already been approved? If it's just a matter of resolving merge conflicts, I can do it

labrinea · 2024-08-07T08:41:13Z

Hey Victor. No, it hasn't been approved. It is still work in progress. Cheers.

As originally discussed in ARM-software#315 and then in ARM-software#329 we want to unify features that the toolchains cannot support independently. In the case of ls64 I have attempted to split the lumped feature in the compiler (see llvm/llvm-project#101712), but unfortunately this would break backwards compatibility: Mapping 'ls64' to FeatureLS64_ACCDATA would enable all three of {FeatureLS64, FeatureLS64_V, FeatureLS64_ACCDATA} but then using 'nols64' either on the command line or the assembler directive would only disable FeatureLS64_ACCDATA without disabling the other two. For that we would have to map 'ls64' to FeatureLS64, but then it would not enable the other two. As far as I am aware there are no hardware implementations out there which implement ls64 without ls64_v or ls64_accdata, so unifying these features in the specification should not be a regression for feature detection. If any of this changes in the feature we can revisit the specification.

As originally discussed in #315 and then in #329 we want to unify features that the toolchains cannot support independently. In the case of ls64 I have attempted to split the lumped feature in the compiler (see llvm/llvm-project#101712), but unfortunately this would break backwards compatibility: Mapping 'ls64' to FeatureLS64_ACCDATA would enable all three of {FeatureLS64, FeatureLS64_V, FeatureLS64_ACCDATA} but then using 'nols64' either on the command line or the assembler directive would only disable FeatureLS64_ACCDATA without disabling the other two. For that we would have to map 'ls64' to FeatureLS64, but then it would not enable the other two. As far as I am aware there are no hardware implementations out there which implement ls64 without ls64_v or ls64_accdata, so unifying these features in the specification should not be a regression for feature detection. If any of this changes in the feature we can revisit the specification.

Currently we maintain a hand written list of subtarget features which we are implied for a given FMV feature. It is more robust to expand such dependencies using ExtensionDependency from TargetParser, since that is generated by tablegen. For this to work each FMV feature must have a corresponding SubtargetFeature in place. There are a few FMV features which don't satisfy this criteria. We are reviewing them in the ACLE specification. I have also added the missing dependences: * FEAT_DPB2 -> FEAT_DPB * FEAT_FlagM2 -> FEAT_FlagM Blocked on ARM-software/acle#315

jroelofs · 2024-10-31T14:43:34Z

Features that enable existing instruction behaviour to be changed when a system register flag is set: the function multiversioning resolvers don't check for runtime enablement of the control flags, so this isn't a suitable way of exploiting the behaviour enabled by those flags.

In the case of bti this conflates the status of the flag with the test for the presence of the ISA feature. You're right that FMV isn't appropriate for a status check, but it is useful for the ISA feature presence check, which for dit you need around your msr dit, x0's. I'm not sure about memtag4 and/or ebf16.

…t. (#113281) Currently we maintain a hand written list of subtarget features which we are implied for a given FMV feature. It is more robust to expand such dependencies using ExtensionDependency from TargetParser, since that is generated by tablegen. For this to work each FMV feature must have a corresponding SubtargetFeature in place. FMV features which didn't satisfy this criteria have been removed from the ACLE specification (ARM-software/acle#315). However, I deliberately marked the ArchExtKind in FMVInfo structure as std::optional in case we decide to break this rule in the future. I have also added the missing dependencies: * FEAT_DPB2 -> FEAT_DPB * FEAT_FlagM2 -> FEAT_FlagM

andrewcarlotti force-pushed the fmv-feature-removal branch from dec82a1 to 1d0e035 Compare April 3, 2024 23:25

labrinea mentioned this pull request Apr 11, 2024

[FMV][GlobalOpt] Statically resolve calls to versioned functions. llvm/llvm-project#87939

Open

labrinea added a commit to labrinea/llvm-project that referenced this pull request Apr 16, 2024

[FMV] Remove useless features according the latest ACLE spec.

9c2dc32

As explained in ARM-software/acle#315 we are deprecating features which aren't adding any value.

labrinea mentioned this pull request Apr 16, 2024

[FMV] Remove useless features according the latest ACLE spec. llvm/llvm-project#88965

Merged

labrinea mentioned this pull request Apr 17, 2024

[FMV] Rectify incomplete ExtensionInfo entries in TargetParser. llvm/llvm-project#89106

Closed

labrinea added a commit to labrinea/llvm-test-suite that referenced this pull request Apr 18, 2024

[FMV] Remove unsupported tests.

870cb0b

In ACLE we are deprecating a bunch of FMV features (see ARM-software/acle#315), so in this patch I am removing the corresponding tests.

labrinea mentioned this pull request Apr 18, 2024

[FMV] Remove unsupported tests. llvm/llvm-test-suite#116

Closed

labrinea mentioned this pull request Apr 18, 2024

Reland "[FMV] Remove useless features according the latest ACLE spec." llvm/llvm-project#89232

Closed

DanielKristofKiss suggested changes Apr 18, 2024

View reviewed changes

labrinea mentioned this pull request Jun 12, 2024

[AArch64] add a target guard to FMV test llvm/llvm-test-suite#133

Merged

labrinea mentioned this pull request Jul 9, 2024

[FMV] Remove features without compiler support and consolidate others. #329

Closed

8 tasks

labrinea mentioned this pull request Sep 4, 2024

[FMV] Unify ls64, ls64_v and ls64_accdata. #346

Merged

8 tasks

vhscampos added the proposal label Sep 12, 2024

labrinea mentioned this pull request Oct 22, 2024

[FMV][AArch64] Expand feature dependencies using AArch64::ExtensionSet. llvm/llvm-project#113281

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove useless function multiversioning features #315

Remove useless function multiversioning features #315

andrewcarlotti commented Mar 28, 2024 •

edited

Loading

labrinea commented Apr 15, 2024

labrinea commented Apr 15, 2024

DanielKristofKiss Apr 18, 2024

andrewcarlotti Apr 23, 2024

andrewcarlotti Apr 23, 2024

DanielKristofKiss commented Apr 18, 2024

andrewcarlotti commented Apr 22, 2024

Wilco1 commented Jun 5, 2024

labrinea commented Jul 9, 2024

DanielKristofKiss commented Jul 9, 2024

vhscampos commented Aug 6, 2024

labrinea commented Aug 7, 2024

jroelofs commented Oct 31, 2024

Remove useless function multiversioning features #315

Are you sure you want to change the base?

Remove useless function multiversioning features #315

Conversation

andrewcarlotti commented Mar 28, 2024 • edited Loading

labrinea commented Apr 15, 2024

labrinea commented Apr 15, 2024

DanielKristofKiss Apr 18, 2024

Choose a reason for hiding this comment

andrewcarlotti Apr 23, 2024

Choose a reason for hiding this comment

andrewcarlotti Apr 23, 2024

Choose a reason for hiding this comment

DanielKristofKiss commented Apr 18, 2024

andrewcarlotti commented Apr 22, 2024

Wilco1 commented Jun 5, 2024

labrinea commented Jul 9, 2024

DanielKristofKiss commented Jul 9, 2024

vhscampos commented Aug 6, 2024

labrinea commented Aug 7, 2024

jroelofs commented Oct 31, 2024

andrewcarlotti commented Mar 28, 2024 •

edited

Loading