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: Skip BBJ_CALLFINALLYRET in BasicBlock::VisitEHSuccs #97184

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

jakobbotsch
Copy link
Member

We currently skip yielding EH successors of BBJ_CALLFINALLYRET nodes in BasicBlock::VisitAllSuccs. BasicBlock::VisitEHSuccs should be consistent with this.

Minor diffs from differences in PHI placement expected.

We currently skip yielding EH successors of BBJ_CALLFINALLYRET nodes in
`BasicBlock::VisitAllSuccs`. `BasicBlock::VisitEHSuccs` should be
consistent with this.
@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 18, 2024
@ghost ghost assigned jakobbotsch Jan 18, 2024
@ghost
Copy link

ghost commented Jan 18, 2024

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

Issue Details

We currently skip yielding EH successors of BBJ_CALLFINALLYRET nodes in BasicBlock::VisitAllSuccs. BasicBlock::VisitEHSuccs should be consistent with this.

Minor diffs from differences in PHI placement expected.

Author: jakobbotsch
Assignees: jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @BruceForstall

Diffs. Surprisingly large on win-x86.

@BruceForstall
Copy link
Member

Surprisingly large on win-x86.

Can you characterize why?

I saw a few cases of no longer reloading a register before a tailcall.

@jakobbotsch
Copy link
Member Author

Surprisingly large on win-x86.

Can you characterize why?

I saw a few cases of no longer reloading a register before a tailcall.

Those are BBJ_EHFINALLYRET blocks (I guess they return via pop eax; jmp eax on x86). Liveness is using VisitEHSuccs directly and on x86 we put the BBJ_CALLFINALLY/BBJ_CALLFINALLYRET inside the try-region whose handler we are calling. It means that before this change liveness always sees a cycle in the flowgraph whenever you have a finally, corresponding to "CALLFINALLYRET (EH)-> finally -> ... -> EHFINALLYRET -> CALLFINALLYRET (EH)-> finally -> ...". So liveness thinks anything live into the finally is also live out of the handler and needs to spill/restore a number of locals due to it.

Also saw some GC info diffs; going to run some stress just to be safe.

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr gcstress0x3-gcstress0xc

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jakobbotsch
Copy link
Member Author

Failures look like #96154, #94393, #97103, #97297 and a timeout in GC stress

@jakobbotsch jakobbotsch merged commit 979a94b into dotnet:main Jan 23, 2024
183 of 197 checks passed
@jakobbotsch jakobbotsch deleted the eh-succs-CALLFINALLYRET branch January 23, 2024 11:57
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
…97184)

We currently skip yielding EH successors of BBJ_CALLFINALLYRET nodes in
`BasicBlock::VisitAllSuccs`. `BasicBlock::VisitEHSuccs` should be
consistent with this.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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