Skip to content

[FMV] Add support for more features - #444

Open
Lukacma wants to merge 6 commits into
ARM-software:mainfrom
Lukacma:main
Open

[FMV] Add support for more features#444
Lukacma wants to merge 6 commits into
ARM-software:mainfrom
Lukacma:main

Conversation

@Lukacma

Copy link
Copy Markdown
Contributor

The goal of this patch is to propose new features for function multi versioning. This should allow users to use FMV with newer features.

This patch also fixes a bug in register name for SME2 feature register.

@mgabka

Copy link
Copy Markdown
Contributor

I think it would make sense to split the "bug fix" part into a separate patch.

@AlfieRichardsArmAlfieRichardsArm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM but I think this ought to wait for #441 so this can get its own FMV support version tick

Comment threadmain/acle.md
| sme2p1 | sme2 |
| sme2p2 | sme2p1 |
| sve2p1 | sve2 |
| sve2p2 | sve2p1 |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not a bug, but I do wonder

As sve2p2 requires armv9.5 is it worth (for FMV) implying everything thing that armv9.5 implies? That is not something we do for architecture options but it feels like there may be more to loose with FMV as it is more likely to be enabled via target_version (sve2p2) as opposed to -march-armv9.5-a+sve2p2 or -mcpu?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

My main concern with this is that we can have situations where features added as optional from certain version will be moved and added as optional from earlier architecture versions in later releases.

@Lukacma

Copy link
Copy Markdown
ContributorAuthor

I have updated the patch with more features. If you guys could take another look that would be great!

@mgabka

Copy link
Copy Markdown
Contributor

Could you please rebase this patch? I think you also need to propagate the "The following table lists the architectures feature mapping for AArch64." with new __FUNCTION_MULTI_VERSIONING_SUPPORT_LEVEL value/values

@Lukacma

Copy link
Copy Markdown
ContributorAuthor

Thanks Maciej. Done. I have added it as a september version.

Comment threadmain/acle.md
| sve2p1 | sve2 |
| sve2p2 | sve2p1 |
| sme-f16f16 | sme2 |
| d128 | lse |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be lse128?
At least that is what I see in the compiler:

def FeatureD128 : ExtensionWithMArch<"d128", "D128",
"FEAT_D128, FEAT_LVA3, FEAT_SYSREG128, FEAT_SYSINSTR128",
"Enable Armv9.4-A 128-bit Page Table Descriptors, System Registers "
"and instructions",
[FeatureLSE128]>

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We don't have lse128 feature in FMV as we internally agreed to add features which introduce new builtins and intrinsics. So I went for indirect dependency here as lse128 requires lse.

Comment threadmain/acle.md
| sme-f8f32 | sme2, fp8 |
| ssve-fp8dot4 | sme2, fp8 |
| ssve-fp8fma | sme2, fp8 |
| ssve-bitperm | sme2 |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be sme2,sve-bitperm

def FeatureSSVE_BitPerm : ExtensionWithMArch<"ssve-bitperm", "SSVE_BitPerm", "FEAT_SSVE_BitPerm",
"Enable Armv9.6-A SVE BitPerm support in streaming SVE mode", [FeatureSME2, FeatureSVEBitPerm]>;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We don't have sve-bitperm feature in FMV. We have sve2-bitperm, which enables both sve2 and sve-bitperm feature and is not a dependency of ssve-bitperm.

Comment threadmain/acle.md
| fp8dot4 | fp8 |
| fp8fma | fp8 |
| sme-f8f32 | sme2, fp8 |
| ssve-fp8dot4 | sme2, fp8 |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do new need the ssve- features? I would imagine we could still have them generated with +sme2 and the sve feature. Am I missing something?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure we could them generated like that, but that is only true for cores which have both non-ssve and ssve feature. If core supports only ssve variant that function version wouldn't be emitted.

Comment threadmain/acle.md Outdated
Comment threadmain/acle.md
| `FEAT_SME2` | sme2 | ```ID_AA64PFR1_EL1.SMEver >= 0b0001``` |
| `FEAT_MOPS` | mops | ```ID_AA64ISAR2_EL1.MOPS >= 0b0001``` |
| `FEAT_CSSC` | cssc | ```ID_AA64ISAR2_EL1.CSSC >= 0b0001``` |
| `FEAT_FP8` | fp8 | ```ID_AA64FPFR0_EL1.F8CVT == 0b1``` |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've noticed that for FP8 the reference informs other system registers, should this be checked as well?
The following fields identify the presence of FEAT_FP8:
• ID_AA64FPFR0_EL1.F8CVT. • ID_AA64FPFR0_EL1.F8E4M3. • ID_AA64FPFR0_EL1.F8E5M2.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That is not necessary imo. Reading the spec for the F8CVT:

FEAT_FP8 implements the functionality identified by the value 1.

To me it means that when the value is 1 FEAT_FP8 is implemented

Sign up for freeto 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.

4 participants

@Lukacma@mgabka@CarolineConcatto@AlfieRichardsArm