Skip to content

Conversation

@amanasifkhalid
Copy link
Contributor

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

@ghost ghost 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
Contributor Author

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

@azure-pipelines
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
Contributor 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
Contributor 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
Contributor Author

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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Co-authored-by: Jakob Botsch Nielsen <[email protected]>
@amanasifkhalid
Copy link
Contributor 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
Contributor Author

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

@azure-pipelines
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
Contributor Author

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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@amanasifkhalid
Copy link
Contributor Author

Failures are known.

@amanasifkhalid amanasifkhalid merged commit 67604d3 into dotnet:main Feb 5, 2024
@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