search

ARM-software / acle

Arm C Language Extensions (ACLE)
Other
89 stars 58 forks source link

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

Closed labrinea closed 2 months ago

labrinea commented 2 months ago

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 https://github.com/llvm/llvm-project/pull/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 X those which apply)

labrinea commented 2 months ago

@DanielKristofKiss @jroelofs @tmatheson-arm @andrewcarlotti

andrewcarlotti commented 2 months ago

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.

labrinea commented 2 months ago

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?

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.

How are we going to deal with this in the future if we ever decide to split the feature?

andrewcarlotti commented 2 months ago

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?

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.

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 crypto, although with less quirks and inconsistencies)).

labrinea commented 2 months ago

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?

tmatheson-arm commented 2 months ago

In the CPUFeatures enum which name shall be used to indicate all three

I would say ls64_accdata, but it's an implementation detail (the name is not part of the ABI, only the bit position)

vhscampos commented 2 months ago

@labrinea The PDF rendering is broken in the table line with priority 520. Can you please fix it? You can find the output PDF at the bottom of this page: https://github.com/ARM-software/acle/actions/runs/10703762051 And you can generate the PDF locally using build_with_docker.sh