Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix checks for hardware intrinsics special casing in IL interpreter #38835

Merged
merged 1 commit into from
Jul 7, 2020
Merged

Fix checks for hardware intrinsics special casing in IL interpreter #38835

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/coreclr/src/vm/interpreter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9095,16 +9095,22 @@ void Interpreter::DoCallWork(bool virtualCall, void* thisArg, CORINFO_RESOLVED_T
static ConfigDWORD s_InterpreterHWIntrinsicsIsSupportedFalse;
if (s_InterpreterHWIntrinsicsIsSupportedFalse.val(CLRConfig::INTERNAL_InterpreterHWIntrinsicsIsSupportedFalse) != 0)
{
if (strcmp(methToCall->GetModule()->GetSimpleName(), "System.Private.CoreLib") == 0 &&
#ifdef _DEBUG // GetDebugClassName() is only available in _DEBUG builds
GCX_PREEMP();

// Hardware intrinsics are recognized by name.
const char* namespaceName = NULL;
const char* className = NULL;
const char* methodName = m_interpCeeInfo.getMethodNameFromMetadata((CORINFO_METHOD_HANDLE)methToCall, &className, &namespaceName, NULL);
if (
#if defined(TARGET_X86) || defined(TARGET_AMD64)
strncmp(methToCall->GetClass()->GetDebugClassName(), "System.Runtime.Intrinsics.X86", 29) == 0 &&
strcmp(namespaceName, "System.Runtime.Intrinsics.X86") == 0 &&
#elif defined(TARGET_ARM64)
strncmp(methToCall->GetClass()->GetDebugClassName(), "System.Runtime.Intrinsics.Arm", 29) == 0 &&
strcmp(namespaceName, "System.Runtime.Intrinsics.Arm") == 0 &&
#endif // defined(TARGET_X86) || defined(TARGET_AMD64)
#endif // _DEBUG
strcmp(methToCall->GetName(), "get_IsSupported") == 0)
strcmp(methodName, "get_IsSupported") == 0
Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor

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 strncmps 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.

Copy link
Member

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.

Copy link
Contributor

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.

)
{
GCX_COOP();
DoGetIsSupported();
didIntrinsic = true;
}
Expand Down