Skip to content

JIT: don't mark callees noinline for non-fatal observations with pgo #114821

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

Conversation

AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Apr 18, 2025

Some inline policy decisions are sensitive to the root method having valid PGO data. And sometimes, with invalid PGO data, the decision is that a particular callee can never be a viable inline. If so, the policy will have the runtime mark the method as NoInlining to short-circuit future inlining decisions and save some JIT throughput.

But if the method had valid PGO data the policy may have made a different decision. By marking this method noinline, the JIT will immediately reject all future inline attempts (say from other calling methods with valid PGO data).

For instance, without PGO, the policy may limit the number of basic blocks in a viable callee to 5. But with PGO the policy is willing to consider callees with many more blocks.

In this PR we modify the reporting policy so that if TieredPGO is active, we only ever report back methods with truly fatal inlining issues, not just "informational" or "performance" issues, regardless of the state of the PGO data for the method being jitted.

Some inline policy decisions are sensitive to the root method having valid
PGO data.  And sometimes the decision is that a particular callee can never
be a viable inline.  If so, the policy will have the runtime mark the method
as `NoInlining` to short-circuit future inlining decisions and save some JIT
throughput.

But if the method had valid PGO data the policy may have made a different
decision. By marking this method noinline, the JIT will immediately reject
all future inline attempts (say from other calling methods with valid PGO data).

For instance, without PGO, the policy may limit the number of basic blocks in
a viable callee to 5. But with PGO the policy is willing to consider callees
with many more blocks.

In this PR we modify the reporting policy so that if TieredPGO is active,
we only ever report back methods with truly fatal inlining issues, not just
"informational" or "performance" issues, regardless of the state of the
PGO data for the method being jitted.
@Copilot Copilot AI review requested due to automatic review settings April 18, 2025 18:07
@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 Apr 18, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refines the JIT inline reporting policy when TieredPGO is active, ensuring that methods are only marked as NoInlining for truly fatal inlining issues.

  • Introduces a dynamic PGO check via fgPgoDynamic.
  • Suppresses NoInlining marking for non-fatal observations when TieredPGO is active.

Copy link
Contributor

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

@AndyAyersMS
Copy link
Member Author

@EgorBo PTAL
cc @dotnet/jit-contrib

Not sure how widely this happens.

I noticed String.Concat(string, string) was getting marked as noinline because one of callers had a mismatched PGO schema.

With this PR we still see the inline fail, but we no longer block future attempts

INLINER: during 'fgInline' result 'failed this callee' reason 'too many basic blocks' for 'System.Diagnostics.Tracing.ManifestBuilder:EndEvent():this' calling 'System.String:Concat(System.String,System.String):System.String'
INLINER: Not marking System.String:Concat(System.String,System.String):System.String NOINLINE; pgo active (observation too many basic blocks)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@AndyAyersMS
Copy link
Member Author

@MihuBot

@AndyAyersMS
Copy link
Member Author

@EgorBo ping

Diffs

As far as I know this only applies to PGO compiles, so PMI/SPMI diffs are likely understating diffs.

@EgorBo
Copy link
Member

EgorBo commented Apr 20, 2025

I remember I accidentally broke the noinline propagation logic and that resulted into ~40 benchmarks improvements in dotnet/performance, I reverted it because it was not intentional, let's see how this goes. I presume it might impact TP, though.

@AndyAyersMS
Copy link
Member Author

Failures are timeouts

@AndyAyersMS AndyAyersMS merged commit 34545d7 into dotnet:main Apr 21, 2025
105 of 107 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants