-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: Fix the SVE_ComputeAddress* #104039
Conversation
@dotnet/arm64-contrib |
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
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.
Nit: We could rearrange the ordering in hwinstrinsiclist so that NI_Sve_Compute8BitAddresses
comes first.
Happy as it is.
We cannot, there is an expectation that the intrinsics are ordered alphabetically now so that resolution can be done via a binary search. |
src/coreclr/jit/hwintrinsicarm64.cpp
Outdated
case NI_Sve_Compute32BitAddresses: | ||
return 2; | ||
case NI_Sve_Compute64BitAddresses: | ||
return 4; |
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.
Is this meant to be 4? I thought you had it commented as 3
in the prior commit?
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.
yes, that is a typo. Great catch.
In #103778, we readjusted the ordering, but didn't encode that information in the instruction correctly.
https://github.com/dotnet/runtime/pull/103778/files#diff-3a0ae3f52b3651d14625d26a4ff55d5028a7116a6f8375a6ca13caa44ed8685f