-
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
Fix checks for hardware intrinsics special casing in IL interpreter #38835
Fix checks for hardware intrinsics special casing in IL interpreter #38835
Conversation
#endif // defined(TARGET_X86) || defined(TARGET_AMD64) | ||
#endif // _DEBUG | ||
strcmp(methToCall->GetName(), "get_IsSupported") == 0) | ||
strcmp(methodName, "get_IsSupported") == 0 |
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 going to get nested classes such as: System.Runtime.Intrinsics.Arm.AdvSimd.Arm64.get_IsSupported
?
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.
Likewise, given that DoGetIsSupported()
just returns false
, is there any reason why we aren't just special casing all of these get_IsSupported
functions?
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 going to get nested classes such as: System.Runtime.Intrinsics.Arm.AdvSimd.Arm64.get_IsSupported?
I think these might need to be strncmp
s to handle all those cases correctly and just check the first 29 chars.
is there any reason why we aren't just special casing all of these get_IsSupported functions?
Could you expand what you mean by "special casing" here? I think this is intended to be a temporary solution until a more production quality approach can be designed. See this comment for context on what kinds of cases need to be covered by a more robust solution.
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.
I was just indicating that they were being handled here to explicitly return false.
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.
Gotcha! Yeah, the intention was to special case all the HW intrinsics' get_IsSupported
methods to return false until the interpreter has a more robust solution.
In #38673, @BruceForstall indicated that
EEClass::GetDebugClassName()
in the checks for theget_IsSupported()
methods of hardware intrinsics is only available in_DEBUG
builds. Hence I changed the checks to obtain the method name, class name, and namespace name from the generally availableCEEInfo::getMethodNameeFromMetadata
method.