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: Set edge likelihoods during patchpoint transformation #97897

Merged
merged 6 commits into from
Feb 5, 2024

Conversation

amanasifkhalid
Copy link
Member

Fixes #97892. During patchpoint expansion, make sure to set edge likelihoods for transformed blocks.

@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 2, 2024
@ghost ghost assigned amanasifkhalid Feb 2, 2024
@ghost
Copy link

ghost commented Feb 2, 2024

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

Issue Details

Fixes #97892. During patchpoint expansion, make sure to set edge likelihoods for transformed blocks.

Author: amanasifkhalid
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@amanasifkhalid
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Comment on lines 163 to 164
trueEdge->setLikelihood(0.5);
falseEdge->setLikelihood(0.5);
Copy link
Member

Choose a reason for hiding this comment

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

The test actually passing should be very rare (I believe the threshold is 1/10000). Also, isn't trueEdge always going to be the BBJ_ALWAYS edge inserted by fgSplitBlockAtBeginning above, so the check here is a bit meaningless?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, isn't trueEdge always going to be the BBJ_ALWAYS edge inserted by fgSplitBlockAtBeginning above, so the check here is a bit meaningless?

Ah you're right, so trueEdge should always have an edge likelihood here. I'll remove the branch.

The test actually passing should be very rare (I believe the threshold is 1/10000).

Is there any value to being more precise than 0.1/0.9 for trueEdge/falseEdge's likelihoods?

Copy link
Member

Choose a reason for hiding this comment

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

In tier0? Doubtful. Also 1/10000 is not entirely accurate since I believe multiple patchpoints share the counter, but @AndyAyersMS can correct me if I'm wrong...

Copy link
Member Author

Choose a reason for hiding this comment

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

I see below this snippet that helperBlock inherits 1% of block's weight; should we have the edge likelihoods match that? So trueEdge is 0.99, and falseEdge is 0.01.

Copy link
Member

@jakobbotsch jakobbotsch Feb 2, 2024

Choose a reason for hiding this comment

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

Makes sense to me to be consistent with that. On further look it looks like TC_OnStackReplacement_InitialCounter is 1000. Not sure if the mismatch is intentional or not; in a way, the most accurate guess we could make is probably 1 / <that value>, but feel free to stick with the 1% here.

Copy link
Member

Choose a reason for hiding this comment

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

Overall it probably doesn't matter since (at least for now) we only have patchpoints in unoptimized code. I guess I'd go with the precedent already there and use the 0.99/0.01 split.

@amanasifkhalid
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Co-authored-by: Jakob Botsch Nielsen <[email protected]>
@amanasifkhalid
Copy link
Member Author

amanasifkhalid commented Feb 2, 2024

@AndyAyersMS this isn't wholly related to this change, but I wonder if we should initialize all edge likelihoods to (1.0 / NumSucc()) (maybe in fgLinkBasicBlocks), so that if we encounter an edge without a likelihood, it's because we didn't set it during/after some call to fgAddRefPred in that phase. In the follow-up work to #97740, I'm encountering methods where we initialize the likelihoods using profile data in fgIncorporateProfileData, and then while inlining a method call, we don't initialize the new blocks' edge likelihoods due to a lack of profile data for the inlinee. If we have some guarantee that likelihoods will always be initialized regardless of whether fgIncorporateProfileData can leverage profile data, then I think the logic for setting/updating likelihoods in later phases will be simpler. If our initial guesses for the successor edges of BBJ_COND, BBJ_SWITCH, etc don't match the profile data, then fgIncorporateProfileData can just overwrite them.

What do you think?

@amanasifkhalid
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS
Copy link
Member

@AndyAyersMS this isn't wholly related to this change, but I wonder if we should initialize all edge likelihoods to (1.0 / NumSucc()) (maybe in fgLinkBasicBlocks), so that if we encounter an edge without a likelihood, it's because we didn't set it during/after some call to fgAddRefPred in that phase. In the follow-up work to #97740, I'm encountering methods where we initialize the likelihoods using profile data in fgIncorporateProfileData, and then while inlining a method call, we don't initialize the new blocks' edge likelihoods due to a lack of profile data for the inlinee. If we have some guarantee that likelihoods will always be initialized regardless of whether fgIncorporateProfileData can leverage profile data, then I think the logic for setting/updating likelihoods in later phases will be simpler. If our initial guesses for the successor edges of BBJ_COND, BBJ_SWITCH, etc don't match the profile data, then fgIncorporateProfileData can just overwrite them.

What do you think?

That's more or less what happens in profile synthesis, with the potential of being a bit more clever over time. Right now it is optional and off by default. So you could try turning it on when profile data is absent (just "AssignLikelihoods", not full synthesis, for now).

@amanasifkhalid
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@amanasifkhalid
Copy link
Member Author

Failures are known.

@amanasifkhalid amanasifkhalid merged commit 67604d3 into dotnet:main Feb 5, 2024
167 of 174 checks passed
@amanasifkhalid amanasifkhalid deleted the likelihood-jitstress branch February 5, 2024 19:34
@github-actions github-actions bot locked and limited conversation to collaborators Mar 7, 2024
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.

[jitstress] Assertion failed '!"Inconsistent profile data"'
3 participants