JIT: Don't transform infinite loops in fgOptimizeBranchToEmptyUnconditional

[amanasifkhalid]

Fixes #116701 (comment).

[amanasifkhalid]

/azp run runtime-jit-experimental

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[dotnet-policy-service]

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

[Copilot]

Pull Request Overview

This PR prevents the JIT from folding an unconditional jump into a target that immediately jumps back, avoiding an unintended infinite loop.
Key changes:

• Introduce a new bDestTarget local and disable optimization when it would create a loop.
• Update the removed-block check to use the bDestTarget variable.
• Retain existing try-region and removal logic.

Comments suppressed due to low confidence (1)

src/coreclr/jit/fgopt.cpp:1307

• Consider adding a unit test that covers the case where an empty block jumps back to its predecessor, verifying that fgOptimizeBranchToEmptyUnconditional does not perform the redirection and avoids an infinite loop.

    if (bDestTarget->GetUniqueSucc() == bDest)

[Copilot]

[nitpick] This standalone // line is empty—consider removing it or merging the adjacent comment blocks to improve readability.

Suggested change

	// entering an infinite loop.
	//
	// entering an infinite loop, which would cause issues.

[amanasifkhalid]

The rest of this method follows this convention -- perhaps we ought to unify the comment style at some point.

[amanasifkhalid]

@dotnet/jit-contrib PTAL. No diffs. runtime-jit-experimental passed.

[AndyAyersMS]

I believe this has broken this way before though I can't find any relevant issue.

Any way you can think of to avoid this sort of problem more definitively?

[amanasifkhalid]

I believe this has broken this way before though I can't find any relevant issue.

The caller has a check for bDest unconditionally jumping to itself -- perhaps that was the case you're thinking of?

Any way you can think of to avoid this sort of problem more definitively?

For the infinite loop to trigger, bDest and bDestTarget have to both be empty, so they should both be compactable too. I suppose encoding some invariant into fgUpdateFlowGraph such that the flowgraph is as compact as possible at all times would reduce this case to the single-block infinite loop case, which we already cover. I was planning on revamping our block compaction strategy during flow opts to make it more efficient, but that work will certainly incur diffs.

[amanasifkhalid]

Now that I'm thinking about it, I don't see why we need fgOptimizeBranchToEmptyUnconditional anymore. If we have some block that jumps to an empty BBJ_ALWAYS to somewhere else, we should always be able to compact the intermediate block away. I might be able to do this with minimal churn...

[AndyAyersMS]

If that doesn't pan out we can take this fix and work on a more major revamping next release.

[amanasifkhalid]

@AndyAyersMS the diffs are on the order of tens of thousands of bytes, in both directions. From a quick look, I can see that the different order of transformations during flow opts sometimes leaves branches swapped, churning downstream phases. These look like good opportunities to investigate, but I think we ought to take the easy fix for now.

[AndyAyersMS]

Sounds good.

[amanasifkhalid]

/ba-g Build Analysis not picking up HostActivation.Tests known failure