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

JIT: Use metadata names for SIMD intrinsic method recognition #76786

Merged
merged 8 commits into from
Oct 22, 2022
Merged

JIT: Use metadata names for SIMD intrinsic method recognition #76786

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
40426f0
JIT: Use metadata names for SIMD intrinsic method recognition
jakobbotsch Oct 8, 2022
9f302cb
Reorder checks in impSIMDIntrinsic
jakobbotsch Oct 9, 2022
b21c7c8
Suggested refactoring
jakobbotsch Oct 9, 2022
c208cfa
Avoid unnecessary metadata lookups in getMethodNameFromMetadata
jakobbotsch Oct 9, 2022
cd9bb9a
Fix nested lookup
jakobbotsch Oct 9, 2022
7b08520
Fix repGetMethodNameFromMetadata
jakobbotsch Oct 10, 2022
36bf9f2
Merge branch 'main' of github.com:dotnet/runtime into avoid-eeGetMeth…
jakobbotsch Oct 10, 2022
b15106c
Merge branch 'main' of github.com:dotnet/runtime into avoid-eeGetMeth…
jakobbotsch Oct 11, 2022
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
11 changes: 7 additions & 4 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8967,11 +8967,14 @@ var_types Compiler::impImportCall(OPCODE opcode,
}

#ifdef FEATURE_SIMD
call = impSIMDIntrinsic(opcode, newobjThis, clsHnd, methHnd, sig, mflags, pResolvedToken->token);
if (call != nullptr)
if (isIntrinsic)
{
bIntrinsicImported = true;
goto DONE_CALL;
call = impSIMDIntrinsic(opcode, newobjThis, clsHnd, methHnd, sig, mflags, pResolvedToken->token);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be part of the slightly higher check that already exists?

There isn't any real reason it should be separate or not handled in impIntrinsic...

Copy link
Member Author

@jakobbotsch jakobbotsch Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially tried refactoring this and moving it in to be part of impIntrinsic, but it turned out to be a bit more work than I wanted here. Also wasn't totally sure with the alt-jit check if I could just move it under the existing if.

I am planning to do some more refactoring for this (starting with #76792) so I think I will leave the further clean up to a future PR.

if (call != nullptr)
{
bIntrinsicImported = true;
goto DONE_CALL;
}
}
#endif // FEATURE_SIMD

Expand Down
13 changes: 5 additions & 8 deletions src/coreclr/jit/simd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -956,7 +956,8 @@ const SIMDIntrinsicInfo* Compiler::getSIMDIntrinsicInfo(CORINFO_CLASS_HANDLE* in
if (sig->numArgs == 0)
{
const SIMDIntrinsicInfo* hwAccelIntrinsicInfo = &(simdIntrinsicInfoArray[SIMDIntrinsicHWAccel]);
if ((strcmp(eeGetMethodName(methodHnd, nullptr), hwAccelIntrinsicInfo->methodName) == 0) &&
const char* methodName = info.compCompHnd->getMethodNameFromMetadata(methodHnd, nullptr, nullptr, nullptr);
if ((strcmp(methodName, hwAccelIntrinsicInfo->methodName) == 0) &&
JITtype2varType(sig->retType) == hwAccelIntrinsicInfo->retType)
{
// Sanity check
Expand Down Expand Up @@ -993,7 +994,7 @@ const SIMDIntrinsicInfo* Compiler::getSIMDIntrinsicInfo(CORINFO_CLASS_HANDLE* in
// TODO-Throughput: replace sequential search by binary search by arranging entries
// sorted by method name.
SIMDIntrinsicID intrinsicId = SIMDIntrinsicInvalid;
const char* methodName = eeGetMethodName(methodHnd, nullptr);
const char* methodName = info.compCompHnd->getMethodNameFromMetadata(methodHnd, nullptr, nullptr, nullptr);
for (int i = SIMDIntrinsicNone + 1; i < SIMDIntrinsicInvalid; ++i)
{
if (strcmp(methodName, simdIntrinsicInfoArray[i].methodName) == 0)
Expand Down Expand Up @@ -1855,18 +1856,14 @@ GenTree* Compiler::impSIMDIntrinsic(OPCODE opcode,
unsigned methodFlags,
int memberRef)
{
assert((methodFlags & CORINFO_FLG_INTRINSIC) != 0);

// Exit early if we are not in one of the SIMD types.
if (!isSIMDClass(clsHnd))
{
return nullptr;
}

// Exit early if the method is not a JIT Intrinsic (which requires the [Intrinsic] attribute).
if ((methodFlags & CORINFO_FLG_INTRINSIC) == 0)
{
return nullptr;
}

// Get base type and intrinsic Id
CorInfoType simdBaseJitType = CORINFO_TYPE_UNDEF;
unsigned size = 0;
Expand Down
42 changes: 21 additions & 21 deletions src/coreclr/vm/jitinterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6248,12 +6248,24 @@ const char* CEEInfo::getMethodNameFromMetadata(CORINFO_METHOD_HANDLE ftnHnd, con
} CONTRACTL_END;

const char* result = NULL;
const char* classResult = NULL;
const char* namespaceResult = NULL;
const char* enclosingResult = NULL;

JIT_TO_EE_TRANSITION();

if (className != NULL)
{
*className = NULL;
}

if (namespaceName != NULL)
{
*namespaceName = NULL;
}

if (enclosingClassName != NULL)
{
*enclosingClassName = NULL;
}

MethodDesc *ftn = GetMethod(ftnHnd);
mdMethodDef token = ftn->GetMemberDef();

Expand All @@ -6263,30 +6275,18 @@ const char* CEEInfo::getMethodNameFromMetadata(CORINFO_METHOD_HANDLE ftnHnd, con
IMDInternalImport* pMDImport = pMT->GetMDImport();

IfFailThrow(pMDImport->GetNameOfMethodDef(token, &result));
IfFailThrow(pMDImport->GetNameOfTypeDef(pMT->GetCl(), &classResult, &namespaceResult));
if (className != NULL || namespaceName != NULL)
{
IfFailThrow(pMDImport->GetNameOfTypeDef(pMT->GetCl(), className, namespaceName));
}
// Query enclosingClassName when the method is in a nested class
// and get the namespace of enclosing classes (nested class's namespace is empty)
if (pMT->GetClass()->IsNested())
if (enclosingClassName != NULL && pMT->GetClass()->IsNested())
{
IfFailThrow(pMDImport->GetNameOfTypeDef(pMT->GetEnclosingCl(), &enclosingResult, &namespaceResult));
IfFailThrow(pMDImport->GetNameOfTypeDef(pMT->GetEnclosingCl(), enclosingClassName, namespaceName));
Copy link
Member Author

@jakobbotsch jakobbotsch Oct 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic seems a bit odd to me in terms of the namespaceResult, IIUC it will work only for one level of nesting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is designed to handle just enough for the JIT to identify intrinsics. We only need one level of nesting for that.

}
}

if (className != NULL)
{
*className = classResult;
}

if (namespaceName != NULL)
{
*namespaceName = namespaceResult;
}

if (enclosingClassName != NULL)
{
*enclosingClassName = enclosingResult;
}

EE_TO_JIT_TRANSITION();

return result;
Expand Down