ARM-software / acle

Arm C Language Extensions (ACLE)
Other
88 stars 54 forks source link

[FMV] Add __FUNCTION_MULTI_VERSIONING_SUPPORT_LEVEL. #301

Closed DanielKristofKiss closed 2 months ago

DanielKristofKiss commented 7 months ago

Checklist: (mark with X those which apply)

DanielKristofKiss commented 7 months ago

cc: @ilinpv @jroelofs @andrewcarlotti

ilinpv commented 7 months ago

Looks reasonable to me.

jroelofs commented 7 months ago

Ditto. Thank you!

vhscampos commented 7 months ago

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

DanielKristofKiss commented 7 months ago

Hi @vhscampos, One of the reasons: https://github.com/llvm/llvm-project/issues/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 commented 7 months ago

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 https://github.com/llvm/llvm-project/issues/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 commented 7 months ago

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:

which are both orthogonal to the implemented ACLE version.

vhscampos commented 2 months ago

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

works for me

vhscampos commented 2 months ago

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

labrinea commented 2 months ago

Lgtm