Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Use proper context in initClass for GDV #87847

Merged
merged 31 commits into from
Jul 26, 2023
Merged

Use proper context in initClass for GDV #87847

merged 31 commits into from
Jul 26, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jun 20, 2023

Fixes #87597

Although, I am not 100% sure in it, checking CI...

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 20, 2023
@ghost ghost assigned EgorBo Jun 20, 2023
@ghost
Copy link

ghost commented Jun 20, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #87597

Although, I am not 100% sure in it, checking CI...

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@EgorBo EgorBo mentioned this pull request Jun 20, 2023
Closed
@build-analysis build-analysis bot mentioned this pull request Jun 21, 2023
Closed
@runfoapp runfoapp bot mentioned this pull request Jun 21, 2023
Closed
@build-analysis build-analysis bot mentioned this pull request Jun 21, 2023
Open
5 tasks
@EgorBo
Copy link
Member Author

EgorBo commented Jun 22, 2023

Massive diffs are just SPMI artifacts - if initClass VM API is invoked with args it has never invoked before - it returns "DONT_INLINE" (instead of reporting a missing context) here:

runtime/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp

Line 1021 in b23c198

return CORINFO_INITCLASS_DONT_INLINE;

This was referenced Jun 25, 2023
Closed
Closed
@EgorBo EgorBo mentioned this pull request Jun 25, 2023
Merged
@AndyAyersMS
Copy link
Member

We should try and get this fix in soon, and then make a case for back porting it to .NET 7.

This was referenced Jul 19, 2023
Closed
Closed
@MichalStrehovsky
Copy link
Member

Basically, resolveVirtualMethod fails to resolve an interface method over Boolean (primitive/value type) type 😕

In the screenshot, the ToString of decl starts with Unboxing MethodDesc:. This means this is the alternative entrypoint of the method that will first unbox this and then call a method on the valuetype.

It looks like this API is not expecting to see these. Should RyuJIT call getUnboxedEntry on this first? Someone needs to first convert this to the sane variant. I don't have a good idea from this who should do it.

This was referenced Jul 19, 2023
Closed
Closed
@davidwrighton
Copy link
Member

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@davidwrighton
Copy link
Member

/azp run runtime-coreclr crossgen2 outerloop

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 87847 in repo dotnet/runtime

@davidwrighton
Copy link
Member

/azp run runtime-coreclr crossgen2 outerloop

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 87847 in repo dotnet/runtime

@EgorBo
Copy link
Member Author

EgorBo commented Jul 22, 2023

/azp run runtime-coreclr outerloop, runtime-coreclr crossgen2, runtime-extra-platforms, runtime-coreclr pgostress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@EgorBo EgorBo mentioned this pull request Jul 22, 2023
Closed
@EgorBo EgorBo marked this pull request as ready for review July 24, 2023 19:39
@EgorBo EgorBo requested a review from MichalStrehovsky as a code owner July 24, 2023 19:39
@EgorBo
Copy link
Member Author

EgorBo commented Jul 24, 2023

@AndyAyersMS can you please sign it off, it seems to be passing all outerloop jobs

AndyAyersMS
AndyAyersMS approved these changes Jul 24, 2023
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Ok with taking this as is and addressing the comments in a follow-up.

src/coreclr/jit/importercalls.cpp
@@ -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)

src/coreclr/jit/indirectcalltransformer.cpp
@@ -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 •
edited
Loading

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

@EgorBo EgorBo merged commit 7ebf967 into dotnet:main Jul 26, 2023
@EgorBo EgorBo deleted the fix-ravendb-issue branch July 26, 2023 08:42
@EgorBo EgorBo mentioned this pull request Jul 26, 2023
Merged
@jakobbotsch jakobbotsch mentioned this pull request Jul 26, 2023
Closed
@EgorBo
Copy link
Member Author

EgorBo commented Aug 1, 2023 •
edited by cincuranet
Loading

Improvements:

@EgorBo EgorBo mentioned this pull request Aug 3, 2023
Open
@cincuranet cincuranet mentioned this pull request Aug 8, 2023
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@jakobbotsch jakobbotsch jakobbotsch left review comments

@AndyAyersMS AndyAyersMS AndyAyersMS approved these changes

@davidwrighton davidwrighton davidwrighton approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

Assignees

@EgorBo EgorBo

@davidwrighton davidwrighton

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Access Violation in jithelpers with Dynamic PGO enabled
6 participants
@EgorBo @AndyAyersMS @jkotas @MichalStrehovsky @davidwrighton @jakobbotsch