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

Allow instrumentation for methods with explicit tail calls #58632

Merged
merged 7 commits into from
Sep 6, 2021

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Sep 3, 2021

Currently we always promote methods with explicit tails to tier1 (and enable opts even in Debug?) so we never instrument them...

From my understanding, QJFL=1 (which is 0 by default) ideally implies a working OSR so we should not hit potential SO

/cc @AndyAyersMS @jakobbotsch

@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 Sep 3, 2021
@ghost
Copy link

ghost commented Sep 3, 2021

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

Issue Details

I'm not fully sure in this change, but looks like currently we always promote methods with explicit tails to tier1 (and enable opts even in Debug?) so we never instrument them...

From my understanding, QJFL=1 (which is 0 by default) ideally implies a working OSR so we should not hit SO here

/cc @AndyAyersMS @jakobbotsch

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member

jakobbotsch commented Sep 3, 2021

From my understanding, QJFL=1 (which is 0 by default) ideally implies a working OSR so we should not hit potential SO

I think the comment is just outdated, we will never hit potential SO anymore since tailcall helpers work in tier-0 as well.
I think it should be ok to always start them out in tier 0 when QJFL=1, but guarding on instrumentation as well seems ok.

EDIT: It should even be ok to always start them out in tier-0 now since we don't optimize recursion to loops in tier0 anyway.

{
// Method has an explicit tail call that may run like a loop or may not be generated as a tail
// call in tier 0, switch to optimized to avoid spending too much time running slower code and
// to avoid stack overflow from recursion
// call in tier 0, switch to optimized to avoid spending too much time running slower code
fgSwitchToOptimized();
Copy link
Member

Choose a reason for hiding this comment

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

On second thought it might be a good idea to go back to what you had before -- jitting these requires creating up to two IL stubs for each explicit tailcall in the function, and those stubs are not currently shared, so the rejit will always recreate them.

@JulieLeeMSFT JulieLeeMSFT added this to the 7.0.0 milestone Sep 3, 2021
@EgorBo EgorBo merged commit 32df679 into dotnet:main Sep 6, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Oct 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants