Skip to content

JIT: tail merge returns with multiple statements - #109670

Merged
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:TailMergeMultistatmentReturns
Nov 11, 2024
Merged

AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:TailMergeMultistatmentReturns

Conversation

@AndyAyersMS

Copy link
Copy Markdown
Member

Remove the restriction that a mergeable return have one statement.

Remove the restriction that a mergeable return have one statement.
@AndyAyersMS
AndyAyersMS requested a review from EgorBo November 9, 2024 16:50
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 9, 2024
@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.

@AndyAyersMS

Copy link
Copy Markdown
Member Author

@EgorBo PTAL
cc @dotnet/jit-contrib

Like #109661, I spotted this when I was experimenting with a very early flow opts pass. Seems to have good diffs. There are some cases now where we'll jump to a ret, but that sort of thing should be handled later on, once we have an idea how big the epilogs can be.

@EgorBo EgorBo 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.

I remember that I wanted to relax it in a follow up PR after that change, but forgot 🙂

@EgorBo

EgorBo commented Nov 9, 2024 •

Copy link
Copy Markdown
Member

the diffs look nice, but I guess the CI failure is related? I don't recall it when I was disabling it.

Looks like tail call related

@AndyAyersMS

Copy link
Copy Markdown
Member Author

the diffs look nice, but I guess the CI failure is related? I don't recall it when I was disabling it.

Looks like tail call related

Yes, they are quite likely related.

@AndyAyersMS

Copy link
Copy Markdown
Member Author

the diffs look nice, but I guess the CI failure is related? I don't recall it when I was disabling it.
Looks like tail call related

Yes, they are quite likely related.

It's because X86 does not set FEATURE_TAILCALL_OPT and so does not set FEATURE_TAILCALL_OPT_SHARED_RETURN and so can't handle having a block with a (nonrecursive) tail call branch to a return block.

Not sure why x86 is special here, but assuming it needs to be this way, I can just use the old logic for x86, or check if the statement before the return is a tail call candidate, and defer merging if so.

FYI @jakobbotsch

@AndyAyersMS

Copy link
Copy Markdown
Member Author

the diffs look nice, but I guess the CI failure is related? I don't recall it when I was disabling it.
Looks like tail call related

Yes, they are quite likely related.

It's because X86 does not set FEATURE_TAILCALL_OPT and so does not set FEATURE_TAILCALL_OPT_SHARED_RETURN and so can't handle having a block with a (nonrecursive) tail call branch to a return block.

Not sure why x86 is special here, but assuming it needs to be this way, I can just use the old logic for x86, or check if the statement before the return is a tail call candidate, and defer merging if so.

FYI @jakobbotsch

I ended up not splitting returns from any tail call as there is a fair bit of twisty logic in morph that expects certain shapes.

@AndyAyersMS
AndyAyersMS merged commit 1ddfa14 into dotnet:main Nov 11, 2024
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
Remove the restriction that a mergeable return have one statement.

But don't split tail calls away from a same-block return.
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 12, 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