-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Optimize Vector64 and Vector128.Create methods #36267
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
Changes from 3 commits
6bc9303
fca5ca5
0a05995
c0b72a5
4d282af
07a08aa
5ec1fcd
b2401b5
e86e2d6
ee58c32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4272,7 +4272,8 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, | |
|
|
||
| if (mustExpand && (retNode == nullptr)) | ||
| { | ||
| NO_WAY("JIT must expand the intrinsic!"); | ||
| assert(!"Unhandled must expand intrinsic, throwing PlatformNotSupportedException"); | ||
| return impUnsupportedHWIntrinsic(CORINFO_HELP_THROW_PLATFORM_NOT_SUPPORTED, method, sig, mustExpand); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously the JIT would fail fast with Now, it will assert in debug mode but will
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
|
|
||
| // Optionally report if this intrinsic is special | ||
|
|
@@ -4509,12 +4510,13 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method) | |
|
|
||
| result = HWIntrinsicInfo::lookupId(this, &sig, className, methodName, enclosingClassName); | ||
| } | ||
| else if (strcmp(methodName, "get_IsSupported") == 0) | ||
| { | ||
| return NI_IsSupported_False; | ||
| } | ||
| else | ||
|
|
||
| if (result == NI_Illegal) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This path accounts for All of the code paths are either directly recursive ( if (AdvSimd.IsSupported)
{
return Vector64.Create(...);
}
return SoftwareFallback(...);So the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit confused about this, as I thought that we always expanded intrinsics, even under minopts.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't necessarily about In both of those scenarios, it is recognized as intrinsic (due to the This was really only a problem for classes like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a comment elaborating as to why here: https://github.com/dotnet/runtime/pull/36267/files#diff-b8d003a58643e5595d2920ca5993b952R4501-R4517 |
||
| { | ||
| if (strcmp(methodName, "get_IsSupported") == 0) | ||
| { | ||
| return NI_IsSupported_False; | ||
| } | ||
| return gtIsRecursiveCall(method) ? NI_Throw_PlatformNotSupportedException : NI_Illegal; | ||
| } | ||
| } | ||
|
|
||
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.
ARM actually exposes native intrinsics for these under the same name as the other primitive types (
vdup_n_s64, etc). I'm going to include this in the misc proposal for APIs that may be missing.