-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
ARM64-SVE: GatherPrefetch #103826
ARM64-SVE: GatherPrefetch #103826
Conversation
Note regarding the
|
1 similar comment
Note regarding the
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
I have a single failure in the testing around invalid value tests. Will investigate and fix Monday. Otherwise ready for review. @dotnet/arm64-contrib @kunalspathak |
@@ -1496,22 +1496,11 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) | |||
{ | |||
assert(hasImmediateOperand); | |||
assert(HWIntrinsicInfo::HasEnumOperand(intrin.id)); | |||
if (intrin.op3->IsCnsIntOrI()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this block as HWIntrinsicImmOpHelper does all those checks for you.
Tests are now all fixed. Issue was to ensure that all invalid values in the tests are actually out of range. (We treat 14 and 15 as in range values because the Arm instruction accepts them). I'm now happy with this PR. (Someone will have to fix up any review comments as I'm away now for a few weeks). |
Thanks for working on it. I will resolve merge conflicts and address any outstanding review comments. |
case NI_Sve_GatherPrefetch64Bit: | ||
case NI_Sve_GatherVector: | ||
case NI_Sve_GatherVectorByteZeroExtend: | ||
case NI_Sve_GatherVectorInt16SignExtend: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems we missed adding this previously when we added other GatherVector
APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
var op1 = Unsafe.Read<{Op1VectorType}<{Op1BaseType}>>(_dataTable.inArray1Ptr); | ||
var op2 = (byte*)_dataTable.inArray2Ptr; | ||
var op3 = Unsafe.Read<{Op3VectorType}<{Op3BaseType}>>(_dataTable.inArray3Ptr); | ||
{Isa}.{Method}(op1, op2, op3, {ValidPrefetch}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@a74nh - while testing something else, I realized that we are not validating the result in these tests.
This reverts commit b0212ca.
* JIT ARM64-SVE: Add Sve.ConditionalExtract* APIs * cleanup: fix formatting * Update Helpers.cs * cleanup: group and place APIs in alphabedical order and align the field * Remove redundant HasScalarInputVariant flags from *Replicate intrinsics * cleanup: place special intrinsics in alphabetical order * cleanup: correctly specify emitted instructions in the comments * fixup! cleanup: place special intrinsics in alphabetical order * fixup! cleanup: group and place APIs in alphabedical order and align the field * fixup! ARM64-SVE: GatherPrefetch (#103826) * Revert "fixup! ARM64-SVE: GatherPrefetch (#103826)" This reverts commit b0212ca. * cleanup: align the lines for the newly added tests. --------- Co-authored-by: Kunal Pathak <[email protected]>
Prefetch versions of the gather APIs. Essentially the same as gatherVector but has an extra enum and does not return anything.