Skip to content

JIT: don't let tail-merge try and remove the OSR-protected entry block#131164

Open
AndyAyersMS wants to merge 1 commit into
dotnet:mainfrom
AndyAyersMS:fix-131056
Open

JIT: don't let tail-merge try and remove the OSR-protected entry block#131164
AndyAyersMS wants to merge 1 commit into
dotnet:mainfrom
AndyAyersMS:fix-131056

Conversation

@AndyAyersMS

Copy link
Copy Markdown
Member

Fixes #131056

Fixes dotnet#131056

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 21, 2026 18:11
@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 21, 2026
@AndyAyersMS

Copy link
Copy Markdown
Member Author

@jakobbotsch PTAL
fyi @dotnet/jit-contrib @BoyBaykiller

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@AndyAyersMS
AndyAyersMS requested a review from jakobbotsch July 21, 2026 18:11
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 prevents fgHeadTailMerge from removing the OSR-protected original entry block (fgEntryBB) while it still has an artificial bbRefs increment applied during import. This avoids hitting fgRemoveBlock assertions that require removed blocks to have bbRefs == 0 and bbPreds == nullptr.

Changes:

  • Add an OSR-specific guard in tail-merge block removal to skip removing fgEntryBB while it is still tracked as the “original method entry”.
  • Add an explanatory comment documenting the OSR artificial bbRefs bump / unprotect sequence.

@AndyAyersMS AndyAyersMS added the Priority:1 Work that is critical for the release, but we could probably ship without label Jul 21, 2026
@BoyBaykiller

Copy link
Copy Markdown
Contributor

Thank you for the fix.
I didn't know you'd have to be so careful when removing blocks so the origional PR caused quite a few asserts so far.
And as far as I am concerned there could be more special cases that I am unaware of and could cause issues...

@JulieLeeMSFT

Copy link
Copy Markdown
Member

Rerunning failed build.

Comment thread src/coreclr/jit/fgopt.cpp
Comment on lines +5156 to +5162
// Try to remove emptyBlock and make its preds jump directly to newTarget.
// Under OSR, the original method entry (fgEntryBB) has an artificial bbRefs
// bump to keep it live until morph un-protects it; removing it here would
// leave that ref dangling and trip asserts in fgRemoveBlock.
//
bool canRemove =
!emptyBlock->HasFlag(BBF_DONT_REMOVE) && (emptyBlock != fgFirstBB) && (emptyBlock != fgOSREntryBB);
bool canRemove = !emptyBlock->HasFlag(BBF_DONT_REMOVE) && (emptyBlock != fgFirstBB) &&
(emptyBlock != fgOSREntryBB) && (!opts.IsOSR() || (emptyBlock != fgEntryBB));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a nit, but I assume this could just be emptyBlock != fgEntryBB, isn't fgEntryBB always null outside OSR?

@jakobbotsch jakobbotsch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, feel free to ignore

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 Priority:1 Work that is critical for the release, but we could probably ship without

Projects

None yet

5 participants