JIT ARM64-SVE: Add Sve.LoadVector*FirstFaulting APIs #104964
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ectorFirstFaulting.
…ly without the FirstFaulting test. Added SveFfrTest template.
|
Note regarding the |
ghost
added
|
Note regarding the |
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
|
@tannergooding, @dotnet/arm64-contrib, this is ready for code review. |
JulieLeeMSFT
changed the title
kunalspathak
reviewed
Aug 7, 2024
kunalspathak
reviewed
Aug 7, 2024
| TestLibrary.TestFramework.BeginScenario(nameof(ConditionalSelect_ZeroOp)); | ||
| ConditionalSelectScenario(_mask, ref _fld1, {Op1VectorType}<{RetBaseType}>.Zero); | ||
| ConditionalSelectScenario(_mask, _dataTable.inArray1Ptr, {Op1VectorType}<{RetBaseType}>.Zero); |
There was a problem hiding this comment.
why this change?
It's tied to other changes in the file, let me try to explain:
- In case of this template
{Op1BaseType}and{Op2BaseType}are the types of themaskandaddressarguments accordingly. Prior to this changeValidateConditionalSelectResultused to accept a vector/array of{Op1BaseType}elements as the firstfirstOp(data) argument. This was correct as long as{Op1BaseType}and{Op2BaseType}are same which used to be true any more. This change addsLoadVector*FirstFaultingAPIs for which this doesn't hold true. This explains changes at lines324-339of this file. - Due to the change
1in the signature ofValidateConditionalSelectResult, the second parameter must be passed as{Op2BaseType*}which leads to the change at line279. - Since
{Op1BaseType}and{Op2BaseType}may differ now,_fld1which has{Op1VectorType}<{RetBaseType}>type can't be passes toValidateConditionalSelectResult. To overcome this, this change replace it with_dataTable.inArray1Ptrofvoid *type at lines264-270. (Also_fld1is assigned a return value ofLoadVector*FirstFaultingat line234which is perfectly fine). - Because of
3, the signature ofConditionalSelectScenariowas changed at line274.
Please let me know if you would like to get more details on this or think this should be implemented another way.
This was referenced Aug 8, 2024
kunalspathak
approved these changes
Aug 8, 2024
There was a problem hiding this comment.
LGTM. Can you please confirm all the `FirstFaulting tests, including the one that @TIHan added pass (with stress)? @amanasifkhalid - once we get the confirmation about the tests, this should be ready to merge.
All Output |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contributes to #99957
Adds:
LoadVectorByteZeroExtendFirstFaultingLoadVectorUInt16ZeroExtendFirstFaultingLoadVectorUInt32ZeroExtendFirstFaultingLoadVectorInt16SignExtendFirstFaultingLoadVectorInt32SignExtendFirstFaultingLoadVectorSByteSignExtendFirstFaultingTesting
The added APIs successfully pass stress testing: