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: Don't run optSetBlockWeights when we have PGO data #111764

Merged
merged 2 commits into from
Jan 25, 2025

Conversation

amanasifkhalid
Copy link
Member

Part of #107749. For PGO scenarios, we plan to rely entirely on profile synthesis to propagate profile-derived weights throughout the flowgraph. This is the first step in transitioning away from the existing weight propagation heuristics. We want to keep these around for non-PGO scenarios for now, though ideally, we'll eventually be able to replace these entirely with profile synthesis (plus branch prediction heuristics when we don't have profile data), and consolidate how the profile is maintained for PGO and non-PGO scenarios.

This had few diffs locally, as optSetBlockWeights already avoids modifying profile-derived weights. The diffs seem to stem from BBF_PROF_WEIGHT not being propagated to all blocks when we have profile data (such as during inlining). This doesn't seem concerning, since the next profile repair pass (which I've yet to add) will propagate the flag as needed.

@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 Jan 23, 2025
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

The diffs seem to stem from BBF_PROF_WEIGHT not being propagated to all blocks when we have profile data (such as during inlining). This doesn't seem concerning, since the next profile repair pass (which I've yet to add) will propagate the flag as needed.

I would rather see us strive to get these flags set properly by each phase that manipulates flow. So yet another aspect of pgo post phase checks: if root method has PGO, all blocks must have BBF_PROF_WEIGHT

@amanasifkhalid
Copy link
Member Author

amanasifkhalid commented Jan 24, 2025

I would rather see us strive to get these flags set properly by each phase that manipulates flow. So yet another aspect of pgo post phase checks: if root method has PGO, all blocks must have BBF_PROF_WEIGHT

That sounds like a better plan. I suppose a reasonable goal is to enable BBF_PROF_WEIGHT propagation up to optSetBlockWeights to make this PR zero-diff, then get this checked in (while progressing the flag checks in tandem)?

amanasifkhalid added a commit that referenced this pull request Jan 24, 2025
)

Part of #107749. Prerequisite to #111764 (comment). Add a post-phase check that ensures BBF_PROF_WEIGHT is set for every reachable block when we have profile data. This check revealed only one site -- when incorporating an inlinee's blocks -- where we couldn't rely on normal flow propagation to set the flag.
@amanasifkhalid
Copy link
Member Author

Diffs are smaller this time around; some of these are probably from the bbNum dependency we still have in fgUpdateFlowGraph, since we're skipping block renumbering when we have PGO data.

@amanasifkhalid amanasifkhalid merged commit 38b1419 into dotnet:main Jan 25, 2025
112 checks passed
@amanasifkhalid amanasifkhalid deleted the skip-optSetBlockWeights branch January 25, 2025 13:58
grendello added a commit to grendello/runtime that referenced this pull request Jan 27, 2025
* main: (22 commits)
  Clean up Stopwatch a bit (dotnet#111834)
  JIT: Fix embedded broadcast simd size (dotnet#111638)
  Revert potential UB due to aliasing + more WB removals (dotnet#111733)
  re-enable acceleration of Vector512<long>.op_Multiply (dotnet#111832)
  Handle OSSL 3.4 change to SAN:othername formatting
  JIT: Fix stack allocated arrays for NativeAOT (dotnet#111827)
  JIT: enhance RBO inference for similar compares to constants (dotnet#111766)
  JIT: Don't run optSetBlockWeights when we have PGO data (dotnet#111764)
  [Android] Make sure RuntimeFlavor=CoreCLR when clr subset is specified (dotnet#111821)
  Change empty subject test certificate to include a critical SAN.
  Fix reversed code offsets in GcInfo (dotnet#111792)
  Swap some libraries areas between leads (dotnet#111816)
  Add left-handed spherical and cylindrical billboards (dotnet#109605)
  JIT: revise `optRelopImpliesRelop` to always set `reverseSense` (dotnet#111803)
  Fix Zip64ExtraField handling (dotnet#111802)
  Add build support for Android+CoreCLR (dotnet#110471)
  arm64: Add bic(s) compact encoding (dotnet#111452)
  JIT: Ensure `BBF_PROF_WEIGHT` flag is set when we have PGO data (dotnet#111780)
  Add support for AVX10.2, Add AVX10.2 API surface and template tests (dotnet#111209)
  JIT: Preliminary for enabling inlining late devirted calls (dotnet#111782)
  ...
amanasifkhalid added a commit that referenced this pull request Jan 29, 2025
…111873)

Part of #107749. Follow-up to #111764. fgComputeMissingBlockWeights doesn't modify anything when we have profile data, and fgCalledCount can be derived from the entry block's weight quite simply.
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