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

JIT: add concept of edge likelihood #81738

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

AndyAyersMS
Copy link
Member

Now that FlowEdges are created early and persist, decorate them with likelihood information early, if we have edge-based PGO data.

We don't use the likelihood for anything yet, but I need to get it in circulation so I can start working on refining both initial and subsequent consistency of the data.

Also add a diagnostic checker for likelhood, and a way to enable it. All of this is off by default.

Now that FlowEdges are created early and persist, decorate them with likelihood
information early, if we have edge-based PGO data.

We don't use the likelihood for anything yet, but I need to get it in circulation
so I can start working on refining both initial and subsequent consistency of
the data.

Also add a diagnostic checker for likelhood, and a way to enable it. All of
this is off by default.
@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 Feb 7, 2023
@ghost ghost assigned AndyAyersMS Feb 7, 2023
@ghost
Copy link

ghost commented Feb 7, 2023

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

Issue Details

Now that FlowEdges are created early and persist, decorate them with likelihood information early, if we have edge-based PGO data.

We don't use the likelihood for anything yet, but I need to get it in circulation so I can start working on refining both initial and subsequent consistency of the data.

Also add a diagnostic checker for likelhood, and a way to enable it. All of this is off by default.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

@EgorBo PTAL
cc @dotnet/jit-contrib

This doesn't really affect the jit's behavior yet (other than perhaps a tiny bit of TP/memory and perhaps some new asserts popping up in PGO stress modes), but I'd like to get the groundwork laid for edge likelihoods, and then start enabling the new diagnostic checks so I can drive the work to fix the various issues. I expect there will be quite a few.

Once the maintenance is in good shape, I'll rip out the old edge weight system and replace it with the likelihoods and the jit will start acting on the new data.

@AndyAyersMS
Copy link
Member Author

Related issue #81638 -- with this PR the internal model calls out pseduo edges, and the edge reconstructor relies on that to figure out how to handle various special cases.

The issue with finally returns could be resolved if it was possible to move the flow manipulations done in impImportLeave over to fgNormalizeEH -- not sure if this is worth considering. Note "returns" from finallies will typically look like criticial edges but they're non-splittable.

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

So currently this doesn't touch cases where we create internal blocks (e.g. expand casts) etc? (create artificial edges)

The idea is to get rid of m_edgeWeightMin/Max from edges eventually?

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Feb 7, 2023

So currently this doesn't touch cases where we create internal blocks (e.g. expand casts) etc? (create artificial edges)

The idea is to get rid of m_edgeWeightMin/Max from edges eventually?

Right, this just sets likelihoods on the edges that exist initially. I still have to go through all the places we add/modify edges later on and set likelihoods for those.

@AndyAyersMS
Copy link
Member Author

PGO pipelines don't seem to be clean, but I can at least check this PR doesn't introduce any new problems.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-coreclr pgo, runtime-coreclr pgostress, runtime-coreclr libraries-pgo

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@AndyAyersMS AndyAyersMS mentioned this pull request Feb 8, 2023
44 tasks
@AndyAyersMS
Copy link
Member Author

No diffs; tiny TP imapct.

PGO and libraries PGO failures look similar to recent runs on main. Possibly some new failures in PGO stress but the failures are non-specific so I am going to go ahead and merge.

@AndyAyersMS AndyAyersMS merged commit e80bc41 into dotnet:main Feb 9, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
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