Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FMV] Add __FUNCTION_MULTI_VERSIONING_SUPPORT_LEVEL. #301

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

DanielKristofKiss
Copy link
Contributor

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.

@DanielKristofKiss
Copy link
Contributor Author

cc: @ilinpv @jroelofs @andrewcarlotti

@ilinpv
Copy link
Contributor

ilinpv commented Feb 23, 2024

Looks reasonable to me.

@jroelofs
Copy link

Ditto. Thank you!

@vhscampos
Copy link
Member

Hi @DanielKristofKiss , can you please give more context to this change? Why is it necessary?

@DanielKristofKiss
Copy link
Contributor Author

Hi @vhscampos,
One of the reasons:
llvm/llvm-project#79659

If the macro reports the version then a program code could check the support level. Later if we introduce new features to the spec then we would have a way to detect it.
Also make easier to check feature level support to ( .e.g. arch FeatureX only introduced in 2025 release of ACLE )

@vhscampos
Copy link
Member

I'm not sure this is a good idea. __HAVE_FUNCTION_MULTI_VERSIONING is a name that reflects a binary choice. Even having it as a bitmask would be a stretch (the previously suggested approach in llvm/llvm-project#79659).

If we want to gate things behing conditions such as this:
#if __HAVE_FUNCTION_MULTI_VERSIONING >= __ARM_ACLE_VERSION(2024, 1, 0 )
Then we should just use directly:
#if __ARM_ACLE >= __ARM_ACLE_VERSION(2024, 1, 0 )

@jroelofs
Copy link

I am not sure that using __ARM_ACLE >= __ARM_ACLE_VERSION(2024, 1, 0 ) directly solves the problem that I have:

the presence of compilers in the wild that claim to but don't actually support [FMV] means we don't have a great way to gate usage of this feature via the preprocessor.

For context, in clang, support for FMV depends on:

  • platform support for IFuncs, or an equivalent lowering (which isn't implemented for all platforms)
  • libcompiler_rt being the runtime library

which are both orthogonal to the implemented ACLE version.

@vhscampos
Copy link
Member

vhscampos commented Aug 5, 2024

Sorry for the long delay. I had some discussions internally but the outcome got lost in the process.

I reckon that __HAVE_FUNCTION_MULTI_VERSIONING is a macro whose value should remain a boolean. Its name has the explicit meaning of a boolean piece of information.

For the purpose intended in this change, I recommend that a new macro is created with a name similar to __MULTI_VERSIONING_SUPPORT_LEVEL, with a value derived from the relevant ACLE version:

#define __MULTI_VERSIONING_SUPPORT_LEVEL __ARM_ACLE_VERSION(2024, 1, 0)

@jroelofs
Copy link

jroelofs commented Aug 5, 2024

works for me

@DanielKristofKiss DanielKristofKiss changed the title [FMV] Change the __HAVE_FUNCTION_MULTI_VERSIONING value. [FMV] Add __FUNCTION_MULTI_VERSIONING_SUPPORT_LEVEL. Aug 6, 2024
@vhscampos
Copy link
Member

@labrinea Can you please review this as well once you're able to?

@labrinea
Copy link
Contributor

labrinea commented Aug 7, 2024

Lgtm

main/acle.md Outdated Show resolved Hide resolved
main/acle.md Outdated Show resolved Hide resolved
main/acle.md Outdated Show resolved Hide resolved
Copy link
Member

@vhscampos vhscampos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you

@vhscampos vhscampos merged commit 92bc958 into ARM-software:main Aug 19, 2024
4 checks passed
labrinea added a commit to labrinea/llvm-project that referenced this pull request Sep 16, 2024
Adds __ARM_ACLE_VERSION and __FUNCTION_MULTI_VERSIONING_SUPPORT_LEVEL
as defined here ARM-software/acle#301 and
here ARM-software/acle#302.

Also bumps __ARM_ACLE to 202420.
labrinea added a commit to llvm/llvm-project that referenced this pull request Sep 17, 2024
…108857)

Adds __ARM_ACLE_VERSION and __FUNCTION_MULTI_VERSIONING_SUPPORT_LEVEL
as defined here ARM-software/acle#301 and
here ARM-software/acle#302.

Also bumps __ARM_ACLE to 202420.
tmsri pushed a commit to tmsri/llvm-project that referenced this pull request Sep 19, 2024
…lvm#108857)

Adds __ARM_ACLE_VERSION and __FUNCTION_MULTI_VERSIONING_SUPPORT_LEVEL
as defined here ARM-software/acle#301 and
here ARM-software/acle#302.

Also bumps __ARM_ACLE to 202420.
Copy link
Contributor

@sallyarmneale sallyarmneale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants