[FMV] Unify ls64, ls64_v and ls64_accdata. #346
Conversation
main/acle.md
Outdated
| | 520 | `FEAT_LS64` | ls64 | ```ID_AA64ISAR1_EL1.LS64 >= 0b0001``` | | ||
| | 530 | `FEAT_LS64_V` | ls64_v | ```ID_AA64ISAR1_EL1.LS64 >= 0b0010``` | | ||
| | 540 | `FEAT_LS64_ACCDATA` | ls64_accdata | ```ID_AA64ISAR1_EL1.LS64 >= 0b0011``` | | ||
| | 520 | `FEAT_LS64`,`FEAT_LS64_V`,<br>`FEAT_LS64_ACCDATA`| ls64 | ```ID_AA64ISAR1_EL1.LS64 >= 0b0011``` | |
There was a problem hiding this comment.
| | 520 | `FEAT_LS64`,`FEAT_LS64_V`,<br>`FEAT_LS64_ACCDATA`| ls64 | ```ID_AA64ISAR1_EL1.LS64 >= 0b0011``` | | |
| | 520 | `FEAT_LS64`,`FEAT_LS64_V`,<br>`FEAT_LS64_ACCDATA`| ls64_accdata | ```ID_AA64ISAR1_EL1.LS64 >= 0b0011``` | |
I'm not sure which way to go here but it's worth discussing. Generally FMV keeps the names of the features matching with the architectural FEAT_ names, and the current PR makes ls64 inconsistent with FEAT_LS64_ACCDATA. If we ever did want to add independent FEAT_LS64 and FEAT_LS64_V to FMV, we would have the same problem we have in the compilers where ls64 can't be reassigned from FEAT_LS64_ACCDATA to FEAT_LS64. However I can see that it makes the feature name inconsistent with the existing compiler command line spellings.
There was a problem hiding this comment.
Being inconsistent with the command line flags could be mitigated with a warning like no such feature ls64; did you mean ls64_accdata?
There was a problem hiding this comment.
I think we want to stay aligned with the command line/assembler directive here, otherwise we will create further confusion. But that's my opinion. @andrewcarlotti, any opinion?
There was a problem hiding this comment.
I agree that FMV feature names should match command line and assembler directive names.
|
LGTM. If we do find a convincing argument for splitting the feature then we'll have to revisit this, but a split would need to use different names to the ones proposed in the original (current) spec, for consistency with existing established command line options. |
But how about this issue I mentioned in the description?
How are we going to deal with this in the future if we ever decide to split the feature? |
We can worry about that issue in the future if necessary; it doesn't affect the decisions we need to make now. (However, one possible solution could be to create three new flags, and the existing flag would become an umbrella flag that enables or disables all of them (similarly to |
|
Not direclty an ACLE question, but it relates to this patch. In the CPUFeatures enum which name shall be used to indicate all three {FeatureLS64, FeatureLS64_V, FeatureLS64_ACCDATA}? FEAT_LS64 or FeatureLS64_ACCDATA? For other entries with more than one features listed in ACLE we use the top level dependency. For example on {FEAT_SM3,FEAT_SM4} we use FEAT_SM4. On that ground I would guess FeatureLS64_ACCDATA is the one to keep? |
I would say ls64_accdata, but it's an implementation detail (the name is not part of the ABI, only the bit position) |
Originally I tried spliting these features in the compiler with llvm#101712, but I realized that there's no way to preserve backwards compatibility, therefore we decided to lump those features in the ACLE specification too ARM-software/acle#346. Since there are no hardware implementations out there which implement ls64 without ls64_v or ls64_accdata, this shouldn't be a regression for feature detection.
|
@labrinea The PDF rendering is broken in the table line with priority 520. Can you please fix it? |
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.
| | 520 | `FEAT_LS64` | ls64 | ```ID_AA64ISAR1_EL1.LS64 >= 0b0001``` | | ||
| | 530 | `FEAT_LS64_V` | ls64_v | ```ID_AA64ISAR1_EL1.LS64 >= 0b0010``` | | ||
| | 540 | `FEAT_LS64_ACCDATA` | ls64_accdata | ```ID_AA64ISAR1_EL1.LS64 >= 0b0011``` | | ||
| | 520 | `FEAT_LS64`, `FEAT_LS64_V`, <br> `FEAT_LS64_ACCDATA` | ls64 | ```ID_AA64ISAR1_EL1.LS64 >= 0b0011``` | |
There was a problem hiding this comment.
I think you only need FEAT_LS64_ACCDATA here since that is the correct feature.
In general it is not clear what we mean if we have multiple FEAT_ flags in an entry - all should be set. If we want to keep doing this, maybe we should make this more explicit?
Originally I tried spliting these features in the compiler with #101712, but we decided to lump those features in the ACLE specification (see ARM-software/acle#346). Since there are no hardware implementations out there which implement ls64 without ls64_v or ls64_accdata, this shouldn't be a regression for feature detection.
| @@ -403,6 +403,7 @@ Armv8.4-A [[ARMARMv84]](#ARMARMv84). Support is added for the Dot Product intrin | |||
| * Added `__FUNCTION_MULTI_VERSIONING_SUPPORT_LEVEL` to indicate the support | |||
| level of the [Function Multi Versioning](#function-multi-versioning). | |||
| * Unified Function Multi Versioning features sha1, sha2. | |||
| * Unified Function Multi Versioning features ls64, ls64_v, ls64_accdata. | |||
There was a problem hiding this comment.
missing AND in the list:
ls64, ls64_v, and ls64_accdata.
| @@ -403,6 +403,7 @@ Armv8.4-A [[ARMARMv84]](#ARMARMv84). Support is added for the Dot Product intrin | |||
| * Added `__FUNCTION_MULTI_VERSIONING_SUPPORT_LEVEL` to indicate the support | |||
| level of the [Function Multi Versioning](#function-multi-versioning). | |||
| * Unified Function Multi Versioning features sha1, sha2. | |||
There was a problem hiding this comment.
2 tiny changes.
List of 2 - needs an AND not a comma
List of 3, use Oxford/Serial Comma, see https://confluence.arm.com/pages/viewpage.action?pageId=1589416839
Originally I tried spliting these features in the compiler with llvm#101712, but we decided to lump those features in the ACLE specification (see ARM-software/acle#346). Since there are no hardware implementations out there which implement ls64 without ls64_v or ls64_accdata, this shouldn't be a regression for feature detection.
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.
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
Xthose which apply)PR (do not bother creating the issue if all you want to do is
fixing the bug yourself).
SPDX-FileCopyrightTextlines on topof 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 usecommas to separate gaps, as in
2018-2020, 2022).Copyrightsection of the sources of thespecification 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.
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.
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.
correctness of the result in the PDF output (please refer to the
instructions on how to build the PDFs
locally).
draftversionis set totruein the YAML headerof the sources of the specifications I have modified.
in the README page of the project.