JIT: Reduce heuristics-derived edge likelihoods into throw blocks found by morph

[amanasifkhalid]

Addresses #112913 (comment). If profile synthesis's edge heuristics assign a non-zero likelihood to an edge into a block that morph later determines will throw, the likelihood should be reduced to avoid propagating much of the method's flow into the throw, and deprioritizing blocks that are likely to be hotter. At some point, we should adjust synthesis to be more global, such that a conditional branch into a path that is post-dominated by a throw block is assigned a rare likelihood (FYI @AndyAyersMS, in case you think this is good enough reason to revive #109656). For the time being, this local transformation covers the trivial case.

[Copilot]

Pull Request Overview

This PR reduces the influence of profile-synthesis heuristics on edges that lead into blocks converted to throws by morph. It marks conditional edges in the synthesis phase and then biases their likelihoods away from throw targets after morph.

• Mark true/false successors of BBJ_COND blocks as heuristic-derived in AssignLikelihoods.
• In fgConvertBBToThrowBB, detect those edges and adjust their likelihoods (0.0/1.0 or 0.5/0.5) to favor non-throw paths.
• Extend FlowEdge with a m_heuristicBasedLikelihood flag and accessors.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/coreclr/jit/fgprofilesynthesis.cpp	Calls setHeuristicBased() on both true/false edges before AssignLikelihoodCond to tag them for later logic
src/coreclr/jit/fgbasic.cpp	Adds loop in fgConvertBBToThrowBB to reduce likelihoods on heuristic-based edges into throw blocks
src/coreclr/jit/block.h	Introduces m_heuristicBasedLikelihood, initializes it, and provides isHeuristicBased/setHeuristicBased

Comments suppressed due to low confidence (2)

src/coreclr/jit/fgbasic.cpp:176

• This new loop for adjusting heuristic-based edges into throw blocks isn’t covered by existing tests. Consider adding targeted JIT profile synthesis tests to verify that likelihoods are correctly rebalanced for conditional branches into throw targets.

// Reduce the heuristics-derived edge likelihoods into 'block' to indicate exceptional behavior

src/coreclr/jit/block.h:715

• Add a brief summary comment above m_heuristicBasedLikelihood and the isHeuristicBased/setHeuristicBased methods explaining their purpose and lifecycle (e.g., how they’re reset in clearLikelihood).

bool isHeuristicBased() const

[Copilot]

The assert on otherEdge->isHeuristicBased() may trigger if the opposite edge wasn’t marked heuristic-based, causing a debug-only failure. Instead of asserting, guard the adjustment logic with an if (otherEdge->isHeuristicBased()) check and skip edges that don’t meet the criteria.

Suggested change

	assert(otherEdge->isHeuristicBased());
	// If the predecessor can jump to a non-throw block, bias the likelihoods to that path
	if (!otherEdge->getDestinationBlock()->KindIs(BBJ_THROW))
	{
	predEdge->setLikelihood(0.0);
	otherEdge->setLikelihood(1.0);
	}
	// If both branches are to throw blocks, consider them equally likely
	else
	{
	predEdge->setLikelihood(0.5);
	otherEdge->setLikelihood(0.5);
	}
	profileInconsistent = true;
	// Proceed only if the other edge is heuristic-based
	if (otherEdge->isHeuristicBased())
	{
	// If the predecessor can jump to a non-throw block, bias the likelihoods to that path
	if (!otherEdge->getDestinationBlock()->KindIs(BBJ_THROW))
	{
	predEdge->setLikelihood(0.0);
	otherEdge->setLikelihood(1.0);
	}
	// If both branches are to throw blocks, consider them equally likely
	else
	{
	predEdge->setLikelihood(0.5);
	otherEdge->setLikelihood(0.5);
	}
	profileInconsistent = true;
	}

[amanasifkhalid]

cc @dotnet/jit-contrib, @AndyAyersMS PTAL. Large size increases, particularly from more loop cloning. I suspect the TP regression in benchmarks.run_pgo is largely from more cloning -- let me verify that locally.

[amanasifkhalid]

With loop cloning disabled, the TP regression in benchmarks.run_pgo disappears:

[18:00:04] Total instructions executed by base: 287818105488
[18:00:04] Total instructions executed by diff: 287819220763
[18:00:04] Total instructions executed delta: 1115275 (0.00% of base)

[AndyAyersMS]

Interesting that it has such an impact. Can you spot check a few diffs to see if they make sense?

I don't think we need postdominators for the general case, I think we can do the postorder propagation like I suggested to you privately.

Makes me wonder if things would be even more impactful if we did the right thing during inlining and converted the flow there.

[amanasifkhalid]

I don't think we need postdominators for the general case, I think we can do the postorder propagation like I suggested to you privately.

Right, I took another look at the writeup you shared and gave it a go, since it seemed simple enough. I didn't look at the reference implementation you linked though, so it's possible I'm doing something naive here.

Makes me wonder if things would be even more impactful if we did the right thing during inlining and converted the flow there.

I'm don't think we can easily catch all of the cases morph can recognize. It's pretty trivial to identify and convert blocks with no-return calls with each failed inline, but we'd still be missing all of the cases that require morph's flow adjustments to make it clear a path throws. Considering the next profile repair run isn't until after morph, I don't think we're losing too much by waiting to run this after, too. Once this is in, I can try moving it earlier to see if we gain anything substantial, if you'd like.

[amanasifkhalid]

Not sure if synthesis is the right place for this code; my only motivation for placing this here is to keep the heuristic's likelihood values in one place.

[amanasifkhalid]

ProfileSynthesis::AssignLikelihoods is currently the only place where we set the heuristic-derived flag. There are plenty of sites in other phases where we set likelihoods based on some heuristic (usually next to a TODO suggesting we verify the likelihood makes sense) -- I should probably spot-check these and set the flag there as well. I've yet to see them cause issues with throw likelihoods, so I've left that out of this PR to reduce clutter.

[AndyAyersMS]

For native compiles I used to worry about underestimating cases where an exception postdominated some important computation:

if (p)
{
    for (int i = 0; i < some_big_number; i++)
    {
       important_compute();
    }
    throw some_exception;
}

Given how your algorithm works it seems like it won't propagate rareness into loops though; instead it will make the loop exit likelihood zero and so the stay in loop likelihood "infinite" and then we'll cap it... is that right?

[amanasifkhalid]

instead it will make the loop exit likelihood zero and so the stay in loop likelihood "infinite" and then we'll cap it... is that right?

That's correct. I haven't been able to find an example simple enough such that the only loop exit is into a throw, but I'd expect this change to make the loop look like it iterates forever likelihood-wise. The only way the rareness would propagate into the loop body is if the continuation edge of the loop's test block also flows into a throw -- this would serve as a sink for the loop's weight, similar to how the exit block was previously sinking the weight before this change.

[amanasifkhalid]

I'm going to run a few extra pipelines to ensure I'm keeping the heuristic-derived flag up-to-date.

[amanasifkhalid]

/azp run runtime-coreclr outerloop, runtime-coreclr libraries-pgo

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[amanasifkhalid]

Re: the diffs, this change inspires more loop cloning by making several instances of GDV cloning profitable. I hypothesize much of this is a reversal of #113896, since that change reduced cloning by draining loop flow into throw blocks. benchmarks.run_pgo diffs seem to be inflated by numerous instances of System.Buffers.SharedArrayPool:Trim() experiencing more cloning. With those contexts removed, the size increases in that collection are quite a bit smaller:

Overall (+113,252 bytes)

Collection	Base size (bytes)	Diff size (bytes)	PerfScore in Diffs
benchmarks.run_pgo.windows.x64.checked.mch	60,434,591	+113,252	+99.98%

[amanasifkhalid]

Diffs with cloning disabled, on win-x64:

Overall (+412,450 bytes)

Collection	Base size (bytes)	Diff size (bytes)	PerfScore in Diffs
aspnet.run.windows.x64.checked.mch	73,115,571	+63,487	+123.46%
benchmarks.run_pgo.windows.x64.checked.mch	60,779,358	+122,377	+101.81%
coreclr_tests.run.windows.x64.checked.mch	410,612,067	+34,344	+86.33%
libraries_tests.run.windows.x64.Release.mch	328,990,492	+192,242	+106.08%

Still a big size increase overall. From what I can see, loop inversion is profitable in many more cases now.

[amanasifkhalid]

libraries-pgo failures are #116655. outerloop failures are #116508.

[amanasifkhalid]

/ba-g outerloop test build timed out