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

Sve: Early versions of Intrinsics/Arm/Sve and hwintrinsiclistarm64sve #94606

Closed
wants to merge 1 commit into from

Conversation

a74nh
Copy link
Contributor

@a74nh a74nh commented Nov 10, 2023

These files correspond to the latest vesion of the un-reviewed SVE API. They have all been auto generated.

hwintrinsiclistarm64sve.h cannot be fully autogenerated and will require manual edits.

These files correspond to the latest vesion of the un-reviewed SVE API.
They have all been auto generated.

hwintrinsiclistarm64sve.h cannot be fully autogenerated and will
require manual edits
@a74nh a74nh marked this pull request as draft November 10, 2023 14:52
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 10, 2023
@ghost
Copy link

ghost commented Nov 10, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

Issue Details

These files correspond to the latest vesion of the un-reviewed SVE API. They have all been auto generated.

hwintrinsiclistarm64sve.h cannot be fully autogenerated and will require manual edits.

Author: a74nh
Assignees: -
Labels:

area-System.Runtime.Intrinsics, community-contribution

Milestone: -

@a74nh
Copy link
Contributor Author

a74nh commented Nov 10, 2023

None of this code can be merged until the API is approved, and will require regenerating as it is changed.

Changes required for hwintrinsiclistarm64sve.h:

  • SIMD size is -1 for everything. In once sense that makes sense because the vector size is unknown, but that isn't of much use. Maybe we could add a new flag HW_Flag_element_size which means that SIMD size is the vector element size.
  • Some of the entries have multiple instructions. Eg INS_SVE_CMPGT/INS_SVE_CMPLT. The script does not have enough information to decide which one to use. This will need manually fixing.
  • Some of the HW_Flag_ flags need adding manually later, eg HW_Flag_Commutative.

My current thought is that when ready, this could be committed with every line commented out. Future patches would then uncomment and fix each line.

We also probably want to only do SVE and SVE2 for now, ignoring all the other features.

@a74nh
Copy link
Contributor Author

a74nh commented Nov 10, 2023

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

This looks great.

My current thought is that when ready, this could be committed with every line commented out. Future patches would then uncomment and fix each line.

Sounds good to me.

Maybe we could add a new flag HW_Flag_element_size which means that SIMD size is the vector element size.

I am not sure if I follow the purpose of HW_Flag_element_size. They are and will be different depending on if they are byte, short, etc.

We also probably want to only do SVE and SVE2 for now, ignoring all the other features.

Can you tag all the issues that covers the API that you added in this PR?

Just a thought for future - If we decide to use your tool for future APIs as well, we have an option to just generate those new subset and copy/paste in this file OR, have your tool add just new entries and retain the existing entries (that were manually editted). That's how I did it for my Arm64Encoding tool. BTW, are you planning to open source your tool that generated the APIs as well as hwintrinsiclistarm64sve.h?

// {TYP_BYTE, TYP_UBYTE, TYP_SHORT, TYP_USHORT, TYP_INT, TYP_UINT, TYP_LONG, TYP_ULONG, TYP_FLOAT, TYP_DOUBLE}
// ***************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************************
// Sve
HARDWARE_INTRINSIC(Sve, Abs, -1, 1, true, {INS_SVE_ABS, INS_invalid, INS_SVE_ABS, INS_invalid, INS_SVE_ABS, INS_invalid, INS_SVE_ABS, INS_invalid, INS_SVE_FABS, INS_SVE_FABS} HW_Category_SIMD, HW_Flag_NoFlag)
Copy link
Member

Choose a reason for hiding this comment

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

all the instruction ids are in lowercase, so should be INS_sve_abs.

HARDWARE_INTRINSIC(Sve, CompareGreaterThanOrEqual, -1, 2, true, {INS_SVE_CMPGE, INS_SVE_CMPHS, INS_SVE_CMPGE, INS_SVE_CMPHS, INS_SVE_CMPGE, INS_SVE_CMPHS, INS_SVE_CMPGE, INS_SVE_CMPHS, INS_SVE_FCMGE, INS_SVE_FCMGE} HW_Category_SIMD, HW_Flag_BaseTypeFromFirstArg|HW_Flag_SpecialCodeGen)
HARDWARE_INTRINSIC(Sve, CompareLessThan, -1, 2, true, {INS_SVE_CMPGT/INS_SVE_CMPLT,INS_SVE_CMPHI/INS_SVE_CMPLO,INS_SVE_CMPGT/INS_SVE_CMPLT,INS_SVE_CMPHI/INS_SVE_CMPLO,INS_SVE_CMPGT/INS_SVE_CMPLT,INS_SVE_CMPHI/INS_SVE_CMPLO,INS_SVE_CMPGT, INS_SVE_CMPHI, INS_SVE_FCMGT, INS_SVE_FCMGT} HW_Category_SIMD, HW_Flag_BaseTypeFromFirstArg|HW_Flag_SpecialCodeGen)
HARDWARE_INTRINSIC(Sve, CompareLessThanOrEqual, -1, 2, true, {INS_SVE_CMPGE/INS_SVE_CMPLE,INS_SVE_CMPHS/INS_SVE_CMPLS,INS_SVE_CMPGE/INS_SVE_CMPLE,INS_SVE_CMPHS/INS_SVE_CMPLS,INS_SVE_CMPGE/INS_SVE_CMPLE,INS_SVE_CMPHS/INS_SVE_CMPLS,INS_SVE_CMPGE, INS_SVE_CMPHS, INS_SVE_FCMGE, INS_SVE_FCMGE} HW_Category_SIMD, HW_Flag_BaseTypeFromFirstArg|HW_Flag_SpecialCodeGen)
HARDWARE_INTRINSIC(Sve, CompareNotEqualTo, -1, 2, true, {INS_SVE_CMPNE, INS_SVE_CMPNE, INS_SVE_CMPNE, INS_SVE_CMPNE, INS_SVE_CMPNE, INS_SVE_CMPNE, INS_SVE_CMPNE, INS_SVE_CMPNE, INS_SVE_FCMNE, INS_SVE_FCMNE} HW_Category_SIMD, HW_Flag_BaseTypeFromFirstArg|HW_Flag_SpecialCodeGen)
Copy link
Member

Choose a reason for hiding this comment

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

I see HW_Flag_SpecialCodeGen on certain intrinsics. how did you decide for which intrinsics to put that flat? I prefer to have HW_Flag_NoFlag or at the max HW_Flag_BaseTypeFromFirstArg and let us add other flags while we develop these intrinsics.

Copy link
Member

Choose a reason for hiding this comment

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

Likewise for EncodesExtraTypeArg, I would start with false and then change it to true if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

EncodesExtraTypeArg

This one should be assumed to be needed unless proven otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see HW_Flag_SpecialCodeGen on certain intrinsics. how did you decide for which intrinsics to put that flat?

If there are multiple ways in the API to get to an instruction. But that isn't clear when looking at the table. Probably safer to just remove and checked manually.

Likewise for EncodesExtraTypeArg, I would start with false and then change it to true if needed.

The type columns have INS_INVALID or an instruction. If more than one column is not set to INS_INVALID then I set EncodesExtraTypeArg to true.


/*****************************************************************************/
#ifndef HARDWARE_INTRINSIC
#error Define HARDWARE_INTRINSIC before including this file
Copy link
Member

Choose a reason for hiding this comment

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

Need to add this file in CMakeLists.txt.

@a74nh
Copy link
Contributor Author

a74nh commented Nov 13, 2023

Maybe we could add a new flag HW_Flag_element_size which means that SIMD size is the vector element size.

I am not sure if I follow the purpose of HW_Flag_element_size. They are and will be different depending on if they are byte, short, etc.

Yes, your right. It just should be -1 throughout.

We also probably want to only do SVE and SVE2 for now, ignoring all the other features.

Can you tag all the issues that covers the API that you added in this PR?

Sure. This is everything in #93095 (comment)

Which is...

FEAT_SVE:

FEAT_SVE2:

Other SVE Features:

Just a thought for future - If we decide to use your tool for future APIs as well, we have an option to just generate those new subset and copy/paste in this file OR, have your tool add just new entries and retain the existing entries (that were manually editted). That's how I did it for my Arm64Encoding tool.

Yes, I could fix up the tool to read in the existing files and fill in the gaps.

BTW, are you planning to open source your tool that generated the APIs as well as hwintrinsiclistarm64sve.h?

Happy to open source it. It's in python. And it's a little messy as there is a lot of renaming and fixing up.

@ghost ghost closed this Dec 13, 2023
@ghost
Copy link

ghost commented Dec 13, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 13, 2024
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.Intrinsics community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants