JIT: verify full profile consistency after importation #100869

AndyAyersMS · 2024-04-10T15:08:36Z

Move the full profile check down past the importer. Attempt local repair
for cases where the importer alters BBJ_COND. If that is unable to guarantee
consistency, mark the PGO data as inconsistent.

If the importer alters BBJ_SWITCH don't attempt repair, just mark the profile
as inconsistent.

If in an OSR method the original method entry is a loop header, and that is
not the loop that triggered OSR, mark the profile as inconsistent.

If the importer re-imports a LEAVE, there are still orphaned blocks left from
the first importation, these can mess up profiles. In that case, mark the
profile as inconsistent.

Exempt blocks with EH preds (catches, etc) from inbound checking, as
profile data propagation along EH edges is not modelled.

Modify the post-phase checks to allow either small relative errors or small
absolute errors, so that flow out of EH regions though intermediaries (say
step blocks) does not trip the checker.

Ensure the initial pass of likelihood adjustments pays attention to
throws. And only mark throws as rare in the importer if we have not
synthesized profile data (which may in fact tell us the throw is not cold).

Contributes to #93020

Move the full profile check down past the importer. Attempt local repair in the one place the importer may alter flow. If that is unable to guarantee consistency, mark the PGO data as inconsistent. Exempt blocks with EH preds (catches, etc) from imbound checking, as profile data propagation along EH edges is not modelled. Ensure the initial pass of likelihood adjustments pays attention to throws. And only mark throws as rare in the importer if we have not synthesized profile data (which may in fact tell us the throw is not cold). Contributes to dotnet#93020

dotnet-policy-service · 2024-04-10T15:10:01Z

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

AndyAyersMS · 2024-04-10T15:11:35Z

@amanasifkhalid PTAL
cc @dotnet/jit-contrib

This causes a bit more churn than I expected... not always marking throws as cold in the importer alters inlining decisions in some cases.

The change in fgLinkBasicBlocks is related to another issue we need to address (as a follow-up) -- the "raw" unsynthesized profile weights can sometimes make it through and be used during jitting. In the case I looked at the callee had a profile schema but all profile data was zero. The likelihoods were consistent, so the repair phase left them as is, and the results converged, so the solver was happy.

AndyAyersMS · 2024-04-10T16:24:35Z

Some stats from asp.net collection

202,728 methods have   consistent profiles after profile incorporation (roots and inlinees).
     32 methods have inconsistent profiles after profile incorporation
   5446 methods have inconsistent profiles after importation (not including the 32)

So the vast majority of methods remain consistent through importation (not as impressive as it sounds, because many methods, especially inlinees, are single basic block...).

AndyAyersMS · 2024-04-10T18:42:15Z

Realized this is not doing as much checking as it should, so there could be more changes needed.. let me fix that.

amanasifkhalid

Initial pass LGTM. I'll take another look once you update this.

amanasifkhalid · 2024-04-10T19:26:16Z

src/coreclr/jit/importer.cpp

+                                //
+                                BasicBlock* const target = block->GetTarget();
+                                assert(target->hasProfileWeight());
+                                target->setBBProfileWeight(target->bbWeight + weight);


Maybe this is premature, but do you think we should consider changing the block weight API surface sooner rather than later? It might be useful as we do these profile fixups to have something like incrementBBProfileWeight that asserts the increment amount is positive, and clears the BBF_RUN_RARELY flag (until we decide to decouple the meaning of this flag from BB_ZERO_WEIGHT) -- and maybe doesn't touch the BBF_PROF_WEIGHT flag?

I'm happy to add some new helpers like this, if you think they'd be useful.

Yes, we should do that -- want to get a bit more experience with the updates first, so we can see what patterns are common.

amanasifkhalid · 2024-04-10T19:28:38Z

src/coreclr/jit/importer.cpp

+                                {
+                                    assert(fgProfileWeightsEqual(alternateNewWeight, 0));
+                                }
+                                alternate->setBBProfileWeight(max(0.0, alternateNewWeight));


Ditto my above comment -- maybe something like decrementBBProfileWeight that does these underflow checks.

AndyAyersMS · 2024-04-12T17:13:14Z

@amanasifkhalid take anther look when you can. Changes since your last review:

reenable proper checking
allow small absolute count discrepancies (nominally, from EH) as well as small relative ones. This is the checking counterpart to JIT: revise how synthesis treats handler regions #100899
handle folded switches (by just bailing out)
handle OSR special case (where original method entry was loop header) (by just bailing out)

AndyAyersMS · 2024-04-12T17:14:21Z

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

azure-pipelines · 2024-04-12T17:14:43Z

Azure Pipelines successfully started running 4 pipeline(s).

amanasifkhalid

Changes LGTM -- thanks for summarizing them! I'll take another look once you get to the stress failures.

amanasifkhalid · 2024-04-12T20:11:11Z

src/coreclr/jit/importer.cpp

+                if (!fgPgoSynthesized)
+                {
+                    // Any block with a throw is rarely executed.
+                    block->bbSetRunRarely();


Following up on the planned BasicBlock interface changes, I imagine this pattern will come up enough that we'll want to move the check into bbSetRunRarely (or do some similar cleanup).

AndyAyersMS · 2024-04-12T20:18:06Z

Looks like some optional CI failures to sort through.

AndyAyersMS · 2024-04-12T20:52:46Z

jitstress failure is timeout in BinderTracingTest.ResolutionFlow (~ ##97735)

libraries-pgo failures look unrelated (~ #98292 ?)

pgostress is related, looks like one test case failing across a number of configs, with fully synthesized profile data.

19:19:22.229 Running test: JIT/Regression/JitBlue/GitHub_9651/GitHub_9651/GitHub_9651.dll

Assert failure(PID 3120 [0x00000c30], Thread: 4296 [0x10c8]): Assertion failed '!"Inconsistent profile data"' in 'FinallyReimp:MainX(System.String[]):int' during 'Importation' (IL size 29; hash 0xd0116466; Tier0)

    File: D:\a\_work\1\s\src\coreclr\jit\fgprofile.cpp:5524
    Image: C:\h\w\B54D0997\p\corerun.exe

AndyAyersMS · 2024-04-12T21:02:54Z

In that test we end up re-importing a leave, which creates a dead callfinally/finallyret pair, but we don't clean that up and so downstream code sees some extra bogus preds with profile data after importation:

---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight   IBC [IL range]   [jump]                            [EH region]        [flags]
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1  0                        100    100 [000..008)-> BB03(0.48),BB02(0.52)   ( cond ) T0      try {       i IBC keep
BB02 [0001]  1  0    BB01                 52     52 [008..00F)-> BB04(1)                 (always) T0                  i IBC
BB03 [0002]  1  0    BB01                 48     48 [00F..010)-> BB04(1)                 (always) T0                  i IBC
BB04 [0003]  2  0    BB02,BB03           100    100 [010..016)-> BB09(1)                 (always) T0      }           i IBC
BB09 [0008]  1       BB04                100    100 [???..???)-> BB05(1)                 (callf )                     i IBC internal
BB10 [0009]  1       BB05                100    100 [???..???)-> BB06(1)                 (callfr)                     i IBC internal
BB07 [0006]  0                           100    100 [???..???)-> BB05(1)                 (callf )                     i IBC internal
BB08 [0007]  1       BB05                100    100 [???..???)-> BB06(1)                 (callfr)                     i IBC internal
BB05 [0004]  3     0 BB07,BB09           100.00 100 [016..01A)-> BB08(0.5),BB10(0.5)     (finret)    H0   finally { } i IBC keep
BB06 [0005]  2       BB08,BB10           100    100 [01A..01D)                           (return)                     i IBC
---------------------------------------------------------------------------------------------------------------------------------------------------------------------

Here BB06 sees 200 flow incoming, 100 weight, hence the assert.

Since reimportation of a BBJ_LEAVE is rare, I will just have it bail out as well (that is, set fgPgoConsistent=false). Ideally perhaps we'd undo the extra flow. It gets cleaned up eventually.

amanasifkhalid

LGTM, assuming CI passes. Thanks!

AndyAyersMS · 2024-04-13T00:34:10Z

I don't see any good reason to rerun the optional CI modes.

Move the full profile check down past the importer. Attempt local repair for cases where the importer alters BBJ_COND. If that is unable to guarantee consistency, mark the PGO data as inconsistent. If the importer alters BBJ_SWITCH don't attempt repair, just mark the profile as inconsistent. If in an OSR method the original method entry is a loop header, and that is not the loop that triggered OSR, mark the profile as inconsistent. If the importer re-imports a LEAVE, there are still orphaned blocks left from the first importation, these can mess up profiles. In that case, mark the profile as inconsistent. Exempt blocks with EH preds (catches, etc) from inbound checking, as profile data propagation along EH edges is not modelled. Modify the post-phase checks to allow either small relative errors or small absolute errors, so that flow out of EH regions though intermediaries (say step blocks) does not trip the checker. Ensure the initial pass of likelihood adjustments pays attention to throws. And only mark throws as rare in the importer if we have not synthesized profile data (which may in fact tell us the throw is not cold). Contributes to dotnet#93020

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 10, 2024

dotnet-policy-service bot assigned AndyAyersMS Apr 10, 2024

amanasifkhalid reviewed Apr 10, 2024

View reviewed changes

AndyAyersMS mentioned this pull request Apr 11, 2024

JIT: revise how synthesis treats handler regions #100899

Merged

AndyAyersMS added 5 commits April 11, 2024 07:30

fix check

6e61a50

Merge branch 'main' into BlockWeightConsistency1

18ebada

allow small count discrepancy

823fd99

handle folded switches

2b41492

fix OSR special case where method entry is loop header

69c4a4a

amanasifkhalid reviewed Apr 12, 2024

View reviewed changes

handle reimported leave

748980f

amanasifkhalid approved these changes Apr 12, 2024

View reviewed changes

AndyAyersMS mentioned this pull request Apr 12, 2024

JIT: Flow Graph Modernization and Improved Block Layout #93020

Open

51 tasks

AndyAyersMS merged commit c9bb3e7 into dotnet:main Apr 13, 2024
110 checks passed

This was referenced Apr 16, 2024

[Perf] Windows/x64: 10 Improvements on 4/10/2024 9:33:44 PM dotnet/perf-autofiling-issues#32748

Closed

[Perf] Windows/x64: 4 Improvements on 4/13/2024 7:03:40 AM dotnet/perf-autofiling-issues#32750

Closed

DrewScoggins mentioned this pull request Apr 18, 2024

[Perf] Linux/arm64: 4 Improvements on 4/13/2024 2:55:37 AM dotnet/perf-autofiling-issues#32884

Closed

github-actions bot locked and limited conversation to collaborators May 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: verify full profile consistency after importation #100869

JIT: verify full profile consistency after importation #100869

AndyAyersMS commented Apr 10, 2024 •

edited

dotnet-policy-service bot commented Apr 10, 2024

AndyAyersMS commented Apr 10, 2024

AndyAyersMS commented Apr 10, 2024

AndyAyersMS commented Apr 10, 2024 •

edited

amanasifkhalid left a comment

amanasifkhalid Apr 10, 2024

AndyAyersMS Apr 12, 2024

amanasifkhalid Apr 10, 2024

AndyAyersMS commented Apr 12, 2024

AndyAyersMS commented Apr 12, 2024

azure-pipelines bot commented Apr 12, 2024

amanasifkhalid left a comment

amanasifkhalid Apr 12, 2024

AndyAyersMS commented Apr 12, 2024

AndyAyersMS commented Apr 12, 2024

AndyAyersMS commented Apr 12, 2024

amanasifkhalid left a comment

AndyAyersMS commented Apr 13, 2024

JIT: verify full profile consistency after importation #100869

JIT: verify full profile consistency after importation #100869

Conversation

AndyAyersMS commented Apr 10, 2024 • edited

dotnet-policy-service bot commented Apr 10, 2024

AndyAyersMS commented Apr 10, 2024

AndyAyersMS commented Apr 10, 2024

AndyAyersMS commented Apr 10, 2024 • edited

amanasifkhalid left a comment

Choose a reason for hiding this comment

amanasifkhalid Apr 10, 2024

Choose a reason for hiding this comment

AndyAyersMS Apr 12, 2024

Choose a reason for hiding this comment

amanasifkhalid Apr 10, 2024

Choose a reason for hiding this comment

AndyAyersMS commented Apr 12, 2024

AndyAyersMS commented Apr 12, 2024

azure-pipelines bot commented Apr 12, 2024

amanasifkhalid left a comment

Choose a reason for hiding this comment

amanasifkhalid Apr 12, 2024

Choose a reason for hiding this comment

AndyAyersMS commented Apr 12, 2024

AndyAyersMS commented Apr 12, 2024

AndyAyersMS commented Apr 12, 2024

amanasifkhalid left a comment

Choose a reason for hiding this comment

AndyAyersMS commented Apr 13, 2024

AndyAyersMS commented Apr 10, 2024 •

edited

AndyAyersMS commented Apr 10, 2024 •

edited