ARM-software / acle

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

Remove useless function multiversioning features #315

Open andrewcarlotti opened 8 months ago

andrewcarlotti commented 8 months ago

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:


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 7 months ago

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 commented 7 months ago

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.

DanielKristofKiss commented 7 months ago

"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 commented 7 months ago

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 commented 5 months ago

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

labrinea commented 4 months ago

Ping @DanielKristofKiss @rsandifo-arm

DanielKristofKiss commented 4 months ago

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

vhscampos commented 3 months ago

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 commented 3 months ago

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

jroelofs commented 3 weeks ago
  1. 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.