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.

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

Use proper context in initClass for GDV #87847

Merged
merged 31 commits into from
Jul 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
81df161
Slightly faster atomic increment
EgorBo Jun 13, 2023
145f1b2
Merge branch 'main' of github.com:dotnet/runtime
EgorBo Jun 19, 2023
9c6343f
Merge branch 'main' of github.com:dotnet/runtime
EgorBo Jun 20, 2023
c8e0dd0
use proper context in GDV
EgorBo Jun 20, 2023
bee352f
test
EgorBo Jun 21, 2023
fa14183
test2
EgorBo Jun 22, 2023
c35c337
Test fix
EgorBo Jun 24, 2023
6716059
Update jitinterface.cpp
EgorBo Jun 25, 2023
8babb27
Merge branch 'main' of github.com:dotnet/runtime into fix-ravendb-issue
EgorBo Jun 25, 2023
7ef4563
test 3
EgorBo Jun 25, 2023
05d2660
Merge branch 'main' of github.com:dotnet/runtime into fix-ravendb-issue
EgorBo Jun 30, 2023
632efc9
Test
EgorBo Jun 30, 2023
5347598
Update importercalls.cpp
EgorBo Jun 30, 2023
6ee9280
Merge branch 'main' of github.com:dotnet/runtime into fix-ravendb-issue
EgorBo Jun 30, 2023
39478fa
fix regressions
EgorBo Jun 30, 2023
848d4ff
Update importercalls.cpp
EgorBo Jun 30, 2023
671e7a6
Merge branch 'main' of github.com:dotnet/runtime into fix-ravendb-issue
EgorBo Jul 2, 2023
fdc390a
Merge branch 'main' of github.com:dotnet/runtime into fix-ravendb-issue
EgorBo Jul 2, 2023
19b8e25
revert
EgorBo Jul 3, 2023
7ee1d65
Merge branch 'fix-ravendb-issue' of github.com:EgorBo/runtime-1 into …
EgorBo Jul 13, 2023
3d68462
Merge branch 'main' of github.com:dotnet/runtime into fix-ravendb-issue
EgorBo Jul 13, 2023
0d4fddb
test fix
EgorBo Jul 15, 2023
83462ac
Merge branch 'main' of github.com:dotnet/runtime into fix-ravendb-issue
EgorBo Jul 15, 2023
c49687b
temp fix for r2r
EgorBo Jul 16, 2023
be7b14d
Merge branch 'main' of github.com:dotnet/runtime into fix-ravendb-issue
EgorBo Jul 16, 2023
361d9dd
Revert r2r workaround
EgorBo Jul 17, 2023
8ff5b0a
Tweak behavior of resolveVirtualMethod in the managed compilers to ha…
davidwrighton Jul 18, 2023
bb7009d
Be a bit more permissive around unboxing stubs in checking virtual ov…
davidwrighton Jul 18, 2023
254a843
Fix Unix build break
davidwrighton Jul 19, 2023
fbee1a6
This is a cleaner fix
davidwrighton Jul 21, 2023
a7ee0bf
Merge branch 'main' of github.com:dotnet/runtime into fix-ravendb-issue
EgorBo Jul 22, 2023
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
13 changes: 7 additions & 6 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -7164,12 +7164,13 @@ class Compiler

bool isCompatibleMethodGDV(GenTreeCall* call, CORINFO_METHOD_HANDLE gdvTarget);

void addGuardedDevirtualizationCandidate(GenTreeCall* call,
CORINFO_METHOD_HANDLE methodHandle,
CORINFO_CLASS_HANDLE classHandle,
unsigned methodAttr,
unsigned classAttr,
unsigned likelihood);
void addGuardedDevirtualizationCandidate(GenTreeCall* call,
CORINFO_METHOD_HANDLE methodHandle,
CORINFO_CLASS_HANDLE classHandle,
CORINFO_CONTEXT_HANDLE contextHandle,
unsigned methodAttr,
unsigned classAttr,
unsigned likelihood);

int getGDVMaxTypeChecks()
{
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/jit/fginline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,8 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi
// info.
}

m_compiler->impDevirtualizeCall(call, nullptr, &method, &methodFlags, &context, nullptr,
CORINFO_CONTEXT_HANDLE contextInput = context;
m_compiler->impDevirtualizeCall(call, nullptr, &method, &methodFlags, &contextInput, &context,
isLateDevirtualization, explicitTailCall);
m_madeChanges = true;
}
Expand Down
35 changes: 23 additions & 12 deletions src/coreclr/jit/importercalls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5999,8 +5999,8 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call,
likelyHood += 100 - likelyHood * numExactClasses;
}

addGuardedDevirtualizationCandidate(call, exactMethod, exactCls, exactMethodAttrs, clsAttrs,
likelyHood);
addGuardedDevirtualizationCandidate(call, exactMethod, exactCls, dvInfo.exactContext, exactMethodAttrs,
clsAttrs, likelyHood);
}

if (call->GetInlineCandidatesCount() == numExactClasses)
Expand All @@ -6027,6 +6027,8 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call,
CORINFO_METHOD_HANDLE likelyMethod = likelyMethodes[candidateId];
unsigned likelihood = likelihoods[candidateId];

CORINFO_CONTEXT_HANDLE likelyContext = NULL;

uint32_t likelyClassAttribs = 0;
if (likelyClass != NO_CLASS_HANDLE)
{
Expand Down Expand Up @@ -6062,7 +6064,8 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call,
break;
}

likelyMethod = dvInfo.devirtualizedMethod;
likelyContext = dvInfo.exactContext;
likelyMethod = dvInfo.devirtualizedMethod;
}

uint32_t likelyMethodAttribs = info.compCompHnd->getMethodAttribs(likelyMethod);
Expand Down Expand Up @@ -6130,8 +6133,8 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call,

// Add this as a potential candidate.
//
addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClass, likelyMethodAttribs, likelyClassAttribs,
likelihood);
addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClass, likelyContext, likelyMethodAttribs,
likelyClassAttribs, likelihood);
}
}

Expand All @@ -6156,12 +6159,13 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call,
// 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)
void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call,
CORINFO_METHOD_HANDLE methodHandle,
CORINFO_CLASS_HANDLE classHandle,
CORINFO_CONTEXT_HANDLE contextHandle,
unsigned methodAttr,
unsigned classAttr,
unsigned likelihood)
{
// This transformation only makes sense for delegate and virtual calls
assert(call->IsDelegateInvoke() || call->IsVirtual());
Expand Down Expand Up @@ -6234,6 +6238,7 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call,
pInfo->guardedClassHandle = classHandle;
pInfo->likelihood = likelihood;
pInfo->requiresInstMethodTableArg = false;
pInfo->exactContextHnd = contextHandle;

// If the guarded class is a value class, look for an unboxed entry point.
//
Expand Down Expand Up @@ -6295,8 +6300,14 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode,
{
InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate for GDV");

CORINFO_CONTEXT_HANDLE moreExactContext = call->GetGDVCandidateInfo(candidateId)->exactContextHnd;
if (moreExactContext == NULL)
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 ever NULL?

Copy link
Member Author

Choose a reason for hiding this comment

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

afair, it's null for method-profiling (delegates)

{
moreExactContext = exactContextHnd;
}

// Do the actual evaluation
impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo,
impMarkInlineCandidateHelper(call, candidateId, moreExactContext, exactContextNeedsRuntimeLookup, callInfo,
ilOffset, &inlineResult);
// Ignore non-inlineable candidates
// TODO: Consider keeping them to just devirtualize without inlining, at least for interface
Expand Down
5 changes: 3 additions & 2 deletions src/coreclr/jit/indirectcalltransformer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -862,7 +862,7 @@ class IndirectCallTransformer

JITDUMP("Direct call [%06u] in block " FMT_BB "\n", compiler->dspTreeID(call), block->bbNum);

CORINFO_METHOD_HANDLE methodHnd = call->gtCallMethHnd;
CORINFO_METHOD_HANDLE methodHnd = inlineInfo->guardedMethodHandle;
Copy link
Member

Choose a reason for hiding this comment

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

I think this isn't strictly necessary...?

Consider leaving as is and then asserting that the method that impDevirtualizeCall produces is the one we expected?

Copy link
Member Author

@EgorBo EgorBo Jul 24, 2023

Choose a reason for hiding this comment

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

I'll check with recent David's VM changes, but last time I checked this was an important part to fix asserts. Basically, call->gtCallMethHnd is the initial base method (e.g. IFoo.DoWork), while inlineInfo->guardedMethodHandle is the actual method we're going to inline

Copy link
Member Author

Choose a reason for hiding this comment

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

Just checked - we still assert in both VMs for that. The problem that we pass the base MethodDesc + more precise generic context that belongs to the inlineInfo->guardedMethodHandle

CORINFO_CONTEXT_HANDLE context = inlineInfo->exactContextHnd;
if (clsHnd != NO_CLASS_HANDLE)
{
Expand All @@ -872,7 +872,8 @@ class IndirectCallTransformer
unsigned methodFlags = compiler->info.compCompHnd->getMethodAttribs(methodHnd);
const bool isLateDevirtualization = true;
const bool explicitTailCall = (call->AsCall()->gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0;
compiler->impDevirtualizeCall(call, nullptr, &methodHnd, &methodFlags, &context, nullptr,
CORINFO_CONTEXT_HANDLE contextInput = context;
compiler->impDevirtualizeCall(call, nullptr, &methodHnd, &methodFlags, &contextInput, &context,
isLateDevirtualization, explicitTailCall);
}
else
Expand Down
32 changes: 19 additions & 13 deletions src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1277,6 +1277,10 @@ private bool resolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO* info)
}

MethodDesc decl = HandleToObject(info->virtualMethod);

// Transform from the unboxing thunk to the normal method
decl = decl.IsUnboxingThunk() ? decl.GetUnboxedMethod() : decl;

Debug.Assert(!decl.HasInstantiation);

if ((info->context != null) && decl.OwningType.IsInterface)
Expand Down Expand Up @@ -1369,7 +1373,6 @@ private bool resolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO* info)
#endif
);
}
info->resolvedTokenDevirtualizedUnboxedMethod = default(CORINFO_RESOLVED_TOKEN);
}
else
{
Expand All @@ -1382,26 +1385,29 @@ private bool resolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO* info)
, methodWithTokenImpl
#endif
);
}

if (unboxingStub)
{
info->resolvedTokenDevirtualizedUnboxedMethod = info->resolvedTokenDevirtualizedMethod;
info->resolvedTokenDevirtualizedUnboxedMethod.tokenContext = contextFromMethod(nonUnboxingImpl);
info->resolvedTokenDevirtualizedUnboxedMethod.hMethod = ObjectToHandle(nonUnboxingImpl);
}
else
{
info->resolvedTokenDevirtualizedUnboxedMethod = default(CORINFO_RESOLVED_TOKEN);
}
if (unboxingStub)
{
info->resolvedTokenDevirtualizedUnboxedMethod = info->resolvedTokenDevirtualizedMethod;
info->resolvedTokenDevirtualizedUnboxedMethod.tokenContext = contextFromMethod(nonUnboxingImpl);
info->resolvedTokenDevirtualizedUnboxedMethod.hMethod = ObjectToHandle(nonUnboxingImpl);
}
else
{
info->resolvedTokenDevirtualizedUnboxedMethod = default(CORINFO_RESOLVED_TOKEN);
}

#if READYTORUN
// Testing has not shown that concerns about virtual matching are significant
// Only generate verification for builds with the stress mode enabled
if (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout)
{
ISymbolNode virtualResolutionNode = _compilation.SymbolNodeFactory.CheckVirtualFunctionOverride(methodWithTokenDecl, objType, methodWithTokenImpl);
AddPrecodeFixup(virtualResolutionNode);
if (!methodWithTokenDecl.Method.OwningType.IsValueType || !methodWithTokenImpl.Method.OwningType.IsValueType)
{
ISymbolNode virtualResolutionNode = _compilation.SymbolNodeFactory.CheckVirtualFunctionOverride(methodWithTokenDecl, objType, methodWithTokenImpl);
AddPrecodeFixup(virtualResolutionNode);
}
}
#endif
info->detail = CORINFO_DEVIRTUALIZATION_DETAIL.CORINFO_DEVIRTUALIZATION_SUCCESS;
Expand Down
6 changes: 0 additions & 6 deletions src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1015,12 +1015,6 @@ CorInfoInitClassResult MethodContext::repInitClass(CORINFO_FIELD_HANDLE field,
key.method = CastHandle(method);
key.context = CastHandle(context);

if ((InitClass == nullptr) || (InitClass->GetIndex(key) == -1))
{
// We could try additional inlines with stress modes, just reject them.
return CORINFO_INITCLASS_DONT_INLINE;
}

DWORD value = InitClass->Get(key);
DEBUG_REP(dmpInitClass(key, value));
CorInfoInitClassResult result = (CorInfoInitClassResult)value;
Expand Down