Simplify and unify Vector64/128/256 platform-agnostic intrinsic handling#23028
Simplify and unify Vector64/128/256 platform-agnostic intrinsic handling#23028tannergooding merged 2 commits intodotnet:masterfrom
Conversation
34eb362 to
a24837c
Compare
|
@fiigii, what is the status here? Is it still WIP? Also, I think the linked bug (which is about emitter cleanup) is wrong. |
|
@tannergooding The x86 side is done. ARM64 JIT changes are also finished, but I did not verify it. Do you know how to get JITDump of ARM64 programs without ARM device?
Ah, thanks, fixed! |
|
I think you can use the |
|
Thanks, will try to get done tonight. |
b4d7502 to
0a35bb6
Compare
|
@dotnet-bot test this please |
0a35bb6 to
07cc90c
Compare
|
@dotnet-bot test Ubuntu x64 Formatting |
|
I have checked ARM64 codegen with altjit. This PR is ready for review. @CarolEidt @tannergooding PTAL |
| result = HWIntrinsicInfo::lookupId(className, methodName, enclosingClassName); | ||
| } | ||
| assert(strcmp(namespaceName + 25, ".X86") == 0 || namespaceName[25] == '\0'); | ||
| result = HWIntrinsicInfo::lookupId(className, methodName, enclosingClassName); |
There was a problem hiding this comment.
@CarolEidt, is this going to play well with the need for all IsSupported checks to be done (i.e. for Arm64 intrinsics to explicitly return false on x86).
There was a problem hiding this comment.
I think that will require some separate work. I don't think that the VM even communicates the non-platform intrinsics to the JIT, so changes will be needed there.
There was a problem hiding this comment.
(i.e. for Arm64 intrinsics to explicitly return false on x86).
This is controlled by managed code, e.g., Arm64/Simd.PlatformNotSupported.cs.
There was a problem hiding this comment.
@fiigii, there is an issue (https://github.com/dotnet/coreclr/issues/20379) tracking us needing to recognize the IsSupported flag for all ISAs, even those on unsupported platforms, to ensure that the constant folding always happens (otherwise it is dependent on several other JIT optimizations, like inlining, successfully happening).
There was a problem hiding this comment.
Right. What's awkward is that we don't want System.Runtime.Intrinsics.Arm.Arm64.Simd to be marked as [Intrinsic], but we would like System.Runtime.Intrinsics.Arm.Arm64.Simd.IsSupported to be. I'm not sure whether that would be an issue.
tannergooding
left a comment
There was a problem hiding this comment.
Overall LGTM.
Just a couple nits and a question.
|
Thanks for review, will solve the table format issues soon. |
2e63b8c to
486aa8c
Compare
|
CI tests seem failed by merge conflict, will rebase to try. |
tannergooding
left a comment
There was a problem hiding this comment.
LGTM, thanks for resolving the feedback.
Feel free to tag me when tests are done and we can get this merged
486aa8c to
2b32d41
Compare
2b32d41 to
d97dc20
Compare
|
The traditional CI gets green. @tannergooding can this PR get merged? |
|
Yes, the failures look to be the ones @CarolEidt is addressing in #23558 |
…ing (dotnet#23028) * Simplify and unify Vector64/128/256 platform-agnostic intrinsic handling * Removed unsupported ISAs
…ing (dotnet/coreclr#23028) * Simplify and unify Vector64/128/256 platform-agnostic intrinsic handling * Removed unsupported ISAs Commit migrated from dotnet/coreclr@4a4ba4d
This PR unify
Vector64/128/256platform-agnostic intrinsic recognition to use the table-driven framework instead of the current big if-sequence inimporter.cpp. And more important, we have discussed several times that we want to improve the intrinsic recognition perf, so a unified name lookup is desired.close https://github.com/dotnet/coreclr/issues/22537
cc @CarolEidt @tannergooding