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

Consider fully replacing BBF_RUN_RARELY with the weight of 0 #48778

Open
SingleAccretion opened this issue Feb 25, 2021 · 3 comments
Open

Consider fully replacing BBF_RUN_RARELY with the weight of 0 #48778

SingleAccretion opened this issue Feb 25, 2021 · 3 comments
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Priority:3 Work that is nice to have tenet-performance Performance related issue
Milestone

Comments

@SingleAccretion
Copy link
Contributor

SingleAccretion commented Feb 25, 2021

I see that most code clearing this flag is doing the moral equivalent of the following:

block->bbWeight = weight;
if (weight > 0)
{
    block->bbFlags &= ~BBF_RUN_RARELY;
}

Perhaps it would be feasible to avoid writing this code and just use bbWeight == 0.0f (bbSetRunRarely/isRunRarely) everywhere instead?

cc @AndyAyersMS

Edit: label addition was not intended :(.

category:cq
theme:profile-feedback

@SingleAccretion SingleAccretion added the tenet-performance Performance related issue label Feb 25, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Feb 25, 2021
@SingleAccretion SingleAccretion changed the title Consider fully replacing BBF_RUN_RARELY with profile weight of 0.0f Consider fully replacing BBF_RUN_RARELY with profile weight of 0 Feb 25, 2021
@AndyAyersMS
Copy link
Member

BBF_RUN_RARELY gets set even when we don't have profile data, eg for things like BBJ_THROW blocks.

My ambition is that we always have some form of profile data flowing around, even if we have to synthesize it via heuristics. Once we get to that point, your suggestion (or something similar) makes total sense.

@SingleAccretion
Copy link
Contributor Author

BBF_RUN_RARELY gets set even when we don't have profile data, eg for things like BBJ_THROW blocks.

I see. My thinking was we'd indicate the fact that that the weight (profile-derived or synthetic) is missing by some sentinel value as well (e. g. -1), but if the plan is to always have valid weights, than this change should indeed wait.

@SingleAccretion SingleAccretion changed the title Consider fully replacing BBF_RUN_RARELY with profile weight of 0 Consider fully replacing BBF_RUN_RARELY with the weight of 0 Feb 25, 2021
@AndyAyersMS
Copy link
Member

AndyAyersMS commented Feb 25, 2021

The plan is to always have weights, yes. In the scheme I'm envisioning, zero weights would have special meaning. Synthesis would only produce zeros for things that are truly always rare, like throws.

When we have true profile data, the plan is to blend in a little bit of synthesis data so that we're not overfitting our optimizations to the observed behavior (in a perhaps ideal world if a profile-optimized program fell "off trace" too often we would reoptimize, but we're a ways off from things like that). So even if a block ends up unprofiled it will have a small nonzero weight.

I expect at that point the notion of rare will split into "truly rare" and "cold" and we'll have to sort out conditions for the optimizations currently gated by rare.

@JulieLeeMSFT JulieLeeMSFT added this to Needs Triage in .NET Core CodeGen via automation Mar 2, 2021
@JulieLeeMSFT JulieLeeMSFT moved this from Needs Triage to PGO in .NET Core CodeGen Mar 2, 2021
@JulieLeeMSFT JulieLeeMSFT added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Mar 2, 2021
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Mar 10, 2021
@JulieLeeMSFT JulieLeeMSFT removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Mar 12, 2021
@JulieLeeMSFT JulieLeeMSFT added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Mar 23, 2021
@JulieLeeMSFT JulieLeeMSFT modified the milestones: 6.0.0, Future Mar 29, 2021
@JulieLeeMSFT JulieLeeMSFT added the Priority:3 Work that is nice to have label Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Priority:3 Work that is nice to have tenet-performance Performance related issue
Projects
Development

No branches or pull requests

3 participants