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

Guarded Devirt: multiple type checks #59604

Closed
wants to merge 29 commits into from
Closed

Guarded Devirt: multiple type checks #59604

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
c1c9626
GuardedDevirt: multiple type checks
EgorBo Sep 25, 2021
f58f930
Fix weights
EgorBo Sep 25, 2021
ad6daec
remove redundant block
EgorBo Sep 25, 2021
7d7bbac
fix build
EgorBo Sep 25, 2021
c629fa1
Fix weights
EgorBo Sep 25, 2021
c2cbe9d
jit-format
EgorBo Sep 25, 2021
7495913
Remove gtGuardedDevirtualizationCandidateInfo field
EgorBo Sep 25, 2021
781c2e7
fix inlining for void methods
EgorBo Oct 2, 2021
07e20cc
Fix non-void return type
EgorBo Oct 2, 2021
bcf7b4f
fix assert
EgorBo Oct 2, 2021
81c94af
Disable multi-checks (for CI)
EgorBo Oct 2, 2021
c048c64
fix more asserts
EgorBo Oct 3, 2021
6a3bdff
Merge branch 'main' of https://github.com/dotnet/runtime into gdv-mul…
EgorBo Oct 3, 2021
37ca01f
Fix all existing issues
EgorBo Oct 3, 2021
756dfdb
disable non-void methods for now
EgorBo Oct 3, 2021
a76b01b
Disable GDV-Chain if multi-check is enabled
EgorBo Oct 3, 2021
80c3399
Formatting
EgorBo Oct 3, 2021
b9a1a05
Clean up
EgorBo Oct 3, 2021
db50163
Clean up
EgorBo Oct 3, 2021
8e70304
enable chain opt for monomorphic calls
EgorBo Oct 4, 2021
158b7d4
enable chain opt for monomorphic calls
EgorBo Oct 4, 2021
e6c989a
Update src/coreclr/jit/importer.cpp
EgorBo Oct 4, 2021
616c97f
Address feedback
EgorBo Oct 4, 2021
720be25
Merge branch 'gdv-multiple-1' of https://github.com/EgorBo/runtime-1 …
EgorBo Oct 4, 2021
710c149
Update indirectcalltransformer.cpp
EgorBo Oct 4, 2021
3805cb3
Merge branch 'main' of https://github.com/dotnet/runtime into gdv-mul…
EgorBo Oct 4, 2021
5f68a56
Address feedback
EgorBo Oct 5, 2021
1043290
Address feedback
EgorBo Oct 5, 2021
910c5c9
Address feedback
EgorBo Oct 5, 2021
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
2 changes: 0 additions & 2 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7301,8 +7301,6 @@ class Compiler
void addGuardedDevirtualizationCandidate(GenTreeCall* call,
CORINFO_METHOD_HANDLE methodHandle,
CORINFO_CLASS_HANDLE classHandle,
unsigned methodAttr,
unsigned classAttr,
unsigned likelihood);

bool doesMethodHaveExpRuntimeLookup()
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compmemkind.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ CompMemKindMacro(TailMergeThrows)
CompMemKindMacro(EarlyProp)
CompMemKindMacro(ZeroInit)
CompMemKindMacro(Pgo)
CompMemKindMacro(GDVCandidates)
//clang-format on

#undef CompMemKindMacro
6 changes: 4 additions & 2 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6424,8 +6424,9 @@ GenTreeCall* Compiler::gtNewCallNode(
{
node->gtInlineCandidateInfo = nullptr;
}
node->gtCallLateArgs = nullptr;
node->gtReturnType = type;
node->gtCallLateArgs = nullptr;
node->gtReturnType = type;
node->gtGDVCandidatesCount = 0;

#ifdef FEATURE_READYTORUN
node->gtEntryPoint.addr = nullptr;
Expand Down Expand Up @@ -8309,6 +8310,7 @@ GenTreeCall* Compiler::gtCloneExprCallHelper(GenTreeCall* tree,
{
copy->gtCallMethHnd = tree->gtCallMethHnd;
copy->gtInlineCandidateInfo = nullptr;
copy->gtGDVCandidatesCount = 0;
}

if (tree->fgArgInfo)
Expand Down
9 changes: 5 additions & 4 deletions src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4758,10 +4758,9 @@ struct GenTreeCall final : public GenTree
// only used for CALLI unmanaged calls (CT_INDIRECT)
GenTree* gtCallCookie;
// gtInlineCandidateInfo is only used when inlining methods
InlineCandidateInfo* gtInlineCandidateInfo;
GuardedDevirtualizationCandidateInfo* gtGuardedDevirtualizationCandidateInfo;
ClassProfileCandidateInfo* gtClassProfileCandidateInfo;
void* gtStubCallStubAddr; // GTF_CALL_VIRT_STUB - these are never inlined
InlineCandidateInfo* gtInlineCandidateInfo;
ClassProfileCandidateInfo* gtClassProfileCandidateInfo;
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
void* gtStubCallStubAddr; // GTF_CALL_VIRT_STUB - these are never inlined
CORINFO_GENERIC_HANDLE compileTimeHelperArgumentHandle; // Used to track type handle argument of dynamic helpers
void* gtDirectCallAddress; // Used to pass direct call address between lower and codegen
};
Expand All @@ -4788,6 +4787,8 @@ struct GenTreeCall final : public GenTree
IL_OFFSET gtRawILOffset;
#endif // defined(DEBUG) || defined(INLINE_DATA)

UINT8 gtGDVCandidatesCount;
Copy link
Member

Choose a reason for hiding this comment

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

Does this make a call node bigger?

If we have room for this, do we have enough room to make the union above a tagged union?

Copy link
Member Author

@EgorBo EgorBo Oct 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@AndyAyersMS Not sure I follow, here is the current win-x64 layout (before my changes) in Release /d1reportSingleClassLayoutGenTreeCall:

4>class GenTreeCall	size(144):
4>	+---
4> 0	| +--- (base class GenTree)
4> 0	| | genTreeOps gtOper
4> 1	| | var_types gtType
4> 2	| | gtCSEnum
4> 3	| | gtLIRFlags
4> 4	| | AssertionInfo gtAssertionInfo
4> 6	| | _gtCostEx
4> 7	| | _gtCostSz
4> 8	| | _gtRegNum
4>  	| | <alignment member> (size=3)
4>12	| | GenTreeFlags gtFlags
4>16	| | ValueNumPair gtVNPair
4>24	| | gtRsvdRegs
4>  	| | <alignment member> (size=4)
4>32	| | gtNext
4>40	| | gtPrev
4>	| +---
4>48	| gtCallThisArg
4>56	| gtCallArgs
4>64	| gtCallLateArgs
4>72	| fgArgInfo
4>80	| tailCallInfo
4>80	| CorInfoCallConvExtension unmgdCallConv
4>88	| GenTreeCallFlags gtCallMoreFlags
4>92.	| gtCallType (bitstart=0,nbits=3)
4>92.	| gtReturnType (bitstart=3,nbits=5)
4>  	| <alignment member> (size=3)
4>96	| gtRetClsHnd
4>104	| gtCallCookie
4>104	| gtInlineCandidateInfo
4>104	| gtClassProfileCandidateInfo
4>104	| gtStubCallStubAddr
4>104	| compileTimeHelperArgumentHandle
4>104	| gtDirectCallAddress
4>112	| gtControlExpr
4>120	| gtCallMethHnd
4>120	| gtCallAddr
4>128	| CORINFO_CONST_LOOKUP gtEntryPoint

in theory we can fit the count into those spare 3 bits? (means maxvalue is 7) if the size matters that much - not sure about impact on other archs/os


bool IsHelperCall() const
{
return gtCallType == CT_HELPER;
Expand Down
161 changes: 98 additions & 63 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6902,8 +6902,8 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken)
if (call->IsVirtualStub())
{
JITDUMP("Restoring stub addr %p from guarded devirt candidate info\n",
dspPtr(call->gtGuardedDevirtualizationCandidateInfo->stubAddr));
call->gtStubCallStubAddr = call->gtGuardedDevirtualizationCandidateInfo->stubAddr;
dspPtr(call->gtInlineCandidateInfo->stubAddr));
call->gtStubCallStubAddr = call->gtInlineCandidateInfo->stubAddr;
}
}
}
Expand Down Expand Up @@ -20531,8 +20531,8 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode,
if (call->IsVirtualStub())
{
JITDUMP("Restoring stub addr %p from guarded devirt candidate info\n",
dspPtr(call->gtGuardedDevirtualizationCandidateInfo->stubAddr));
call->gtStubCallStubAddr = call->gtGuardedDevirtualizationCandidateInfo->stubAddr;
dspPtr(call->gtInlineCandidateInfo->stubAddr));
call->gtStubCallStubAddr = call->gtInlineCandidateInfo->stubAddr;
}
}

Expand Down Expand Up @@ -20661,13 +20661,14 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call,

if (call->IsGuardedDevirtualizationCandidate())
{
if (call->gtGuardedDevirtualizationCandidateInfo->guardedMethodUnboxedEntryHandle != nullptr)
// TODO: there can be multiple candidates
if (call->gtInlineCandidateInfo->guardedMethodUnboxedEntryHandle != nullptr)
{
fncHandle = call->gtGuardedDevirtualizationCandidateInfo->guardedMethodUnboxedEntryHandle;
fncHandle = call->gtInlineCandidateInfo->guardedMethodUnboxedEntryHandle;
}
else
{
fncHandle = call->gtGuardedDevirtualizationCandidateInfo->guardedMethodHandle;
fncHandle = call->gtInlineCandidateInfo->guardedMethodHandle;
}
methAttr = info.compCompHnd->getMethodAttribs(fncHandle);
}
Expand Down Expand Up @@ -21902,11 +21903,8 @@ void Compiler::considerGuardedDevirtualization(
// See if there's a likely guess for the class.
//
const unsigned likelihoodThreshold = isInterface ? 25 : 30;
unsigned likelihood = 0;
unsigned numberOfClasses = 0;

CORINFO_CLASS_HANDLE likelyClass = NO_CLASS_HANDLE;

bool doRandomDevirt = false;

const int maxLikelyClasses = 32;
Expand Down Expand Up @@ -21940,17 +21938,12 @@ void Compiler::considerGuardedDevirtualization(

// For now we only use the most popular type

likelihood = likelyClasses[0].likelihood;
likelyClass = likelyClasses[0].clsHandle;

if (numberOfClasses < 1)
{
JITDUMP("No likely class, sorry\n");
return;
}

assert(likelyClass != NO_CLASS_HANDLE);

// Print all likely classes
JITDUMP("%s classes for %p (%s):\n", doRandomDevirt ? "Random" : "Likely", dspPtr(objClass), objClassName)
for (UINT32 i = 0; i < numberOfClasses; i++)
Expand All @@ -21959,44 +21952,71 @@ void Compiler::considerGuardedDevirtualization(
eeGetClassName(likelyClasses[i].clsHandle), likelyClasses[i].likelihood);
}

// Todo: a more advanced heuristic using likelihood, number of
// classes, and the profile count for this block.
//
// For now we will guess if the likelihood is at least 25%/30% (intfc/virt), as studies
// have shown this transformation should pay off even if we guess wrong sometimes.
//
if (likelihood < likelihoodThreshold)
assert(call->gtGDVCandidatesCount == 0);

for (UINT i = 0; i < numberOfClasses; i++)
{
JITDUMP("Not guessing for class; likelihood is below %s call threshold %u\n", callKind, likelihoodThreshold);
return;
}
if (call->gtGDVCandidatesCount >= (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount())
{
break;
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
}

// Figure out which method will be called.
//
CORINFO_DEVIRTUALIZATION_INFO dvInfo;
dvInfo.virtualMethod = baseMethod;
dvInfo.objClass = likelyClass;
dvInfo.context = *pContextHandle;
dvInfo.exactContext = *pContextHandle;
dvInfo.pResolvedTokenVirtualMethod = nullptr;
assert(likelyClasses[i].clsHandle != NO_CLASS_HANDLE);

const bool canResolve = info.compCompHnd->resolveVirtualMethod(&dvInfo);
// Todo: a more advanced heuristic using likelihood, number of
// classes, and the profile count for this block.
//
// For now we will guess if the likelihood is at least 25%/30% (intfc/virt), as studies
// have shown this transformation should pay off even if we guess wrong sometimes.
//
if ((likelyClasses[i].likelihood < likelihoodThreshold / (i + 1)))
{
JITDUMP("Not guessing for class; likelihood is below %s call threshold %u\n", callKind,
likelihoodThreshold / (i + 1));
break;
}

if (!canResolve)
{
JITDUMP("Can't figure out which method would be invoked, sorry\n");
return;
}
// Figure out which method will be called.
//
CORINFO_DEVIRTUALIZATION_INFO dvInfo = {};
dvInfo.virtualMethod = baseMethod;
dvInfo.objClass = likelyClasses[i].clsHandle;
dvInfo.context = *pContextHandle;
dvInfo.exactContext = *pContextHandle;
dvInfo.pResolvedTokenVirtualMethod = nullptr;

CORINFO_METHOD_HANDLE likelyMethod = dvInfo.devirtualizedMethod;
JITDUMP("%s call would invoke method %s\n", callKind, eeGetMethodName(likelyMethod, nullptr));
const bool canResolve = info.compCompHnd->resolveVirtualMethod(&dvInfo);

// Add this as a potential candidate.
//
uint32_t const likelyMethodAttribs = info.compCompHnd->getMethodAttribs(likelyMethod);
uint32_t const likelyClassAttribs = info.compCompHnd->getClassAttribs(likelyClass);
addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClass, likelyMethodAttribs, likelyClassAttribs,
likelihood);
if (!canResolve)
{
JITDUMP("Can't figure out which method would be invoked, sorry\n");
continue;
}

CORINFO_METHOD_HANDLE likelyMethod = dvInfo.devirtualizedMethod;
JITDUMP("%s call would invoke method %s\n", callKind, eeGetMethodName(likelyMethod, nullptr));

if (i > 0)
{
// TODO: skip valuetypes and generics for secondary guesses for now
CORINFO_SIG_INFO sig;
info.compCompHnd->getMethodSig(likelyMethod, &sig);
if ((sig.sigInst.methInstCount != 0) || (sig.sigInst.classInstCount != 0))
{
break;
}

if ((info.compCompHnd->getClassAttribs(likelyClasses[i].clsHandle) & CORINFO_FLG_VALUECLASS) != 0)
{
break;
}
}

// Add this as a potential candidate.
//
addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClasses[i].clsHandle,
likelyClasses[i].likelihood);
}
}

//------------------------------------------------------------------------
Expand All @@ -22016,15 +22036,11 @@ void Compiler::considerGuardedDevirtualization(
// call - potential guarded devirtualization candidate
// methodHandle - method that will be invoked if the class test succeeds
// classHandle - class that will be tested for at runtime
// methodAttr - attributes of the method
// classAttr - attributes of the class
// likelihood - odds that this class is the class seen at runtime
//
void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call,
CORINFO_METHOD_HANDLE methodHandle,
CORINFO_CLASS_HANDLE classHandle,
unsigned methodAttr,
unsigned classAttr,
unsigned likelihood)
{
// This transformation only makes sense for virtual calls
Expand Down Expand Up @@ -22091,17 +22107,17 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call,
// Gather some information for later. Note we actually allocate InlineCandidateInfo
// here, as the devirtualized half of this call will likely become an inline candidate.
//
GuardedDevirtualizationCandidateInfo* pInfo = new (this, CMK_Inlining) InlineCandidateInfo;
InlineCandidateInfo pInfo = {};

pInfo->guardedMethodHandle = methodHandle;
pInfo->guardedMethodUnboxedEntryHandle = nullptr;
pInfo->guardedClassHandle = classHandle;
pInfo->likelihood = likelihood;
pInfo->requiresInstMethodTableArg = false;
pInfo.guardedMethodHandle = methodHandle;
pInfo.guardedMethodUnboxedEntryHandle = nullptr;
pInfo.guardedClassHandle = classHandle;
pInfo.likelihood = likelihood;
pInfo.requiresInstMethodTableArg = false;

// If the guarded class is a value class, look for an unboxed entry point.
//
if ((classAttr & CORINFO_FLG_VALUECLASS) != 0)
if ((info.compCompHnd->getClassAttribs(classHandle) & CORINFO_FLG_VALUECLASS) != 0)
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
{
JITDUMP(" ... class is a value class, looking for unboxed entry\n");
bool requiresInstMethodTableArg = false;
Expand All @@ -22111,8 +22127,8 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call,
if (unboxedEntryMethodHandle != nullptr)
{
JITDUMP(" ... updating GDV candidate with unboxed entry info\n");
pInfo->guardedMethodUnboxedEntryHandle = unboxedEntryMethodHandle;
pInfo->requiresInstMethodTableArg = requiresInstMethodTableArg;
pInfo.guardedMethodUnboxedEntryHandle = unboxedEntryMethodHandle;
pInfo.requiresInstMethodTableArg = requiresInstMethodTableArg;
}
}

Expand All @@ -22121,14 +22137,33 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call,
if (call->IsVirtualStub())
{
JITDUMP("Saving stub addr %p in candidate info\n", dspPtr(call->gtStubCallStubAddr));
pInfo->stubAddr = call->gtStubCallStubAddr;
pInfo.stubAddr = call->gtStubCallStubAddr;
}
else
{
pInfo->stubAddr = nullptr;
pInfo.stubAddr = nullptr;
}

call->gtGuardedDevirtualizationCandidateInfo = pInfo;
const UINT8 maxCandidates = (UINT8)JitConfig.JitGuardedDevirtualizationCheckCount();
const UINT8 candidatesCount = ++call->gtGDVCandidatesCount;
assert(maxCandidates >= candidatesCount);

if (candidatesCount == 1)
{
// in 90% cases virtual calls are monomorphic so let's avoid allocating too much
call->gtInlineCandidateInfo = new (this, CMK_GDVCandidates) InlineCandidateInfo[1]{pInfo};
}
else if (candidatesCount == 2)
{
// Ok, re-alloc to store multiple candidates.
InlineCandidateInfo firstInfo = call->gtInlineCandidateInfo[0];
call->gtInlineCandidateInfo =
new (this, CMK_GDVCandidates) InlineCandidateInfo[maxCandidates]{firstInfo, pInfo};
}
else
{
call->gtInlineCandidateInfo[candidatesCount - 1] = pInfo;
}
}

void Compiler::addExpRuntimeLookupCandidate(GenTreeCall* call)
Expand Down
Loading