Skip to content

JIT: Switch tailcall implicit byref optimization to use last use information - #81033

Merged
jakobbotsch merged 8 commits into
dotnet:mainfrom
jakobbotsch:last-use-tailcalls
Jan 27, 2023
Merged

JIT: Switch tailcall implicit byref optimization to use last use information#81033
jakobbotsch merged 8 commits into
dotnet:mainfrom
jakobbotsch:last-use-tailcalls

Conversation

@jakobbotsch

Copy link
Copy Markdown
Member

The last use information is more general in most cases (for example, the
old logic cannot allow the optimization when there are previous field
writes to the implicit byref). In addition the old logic can interact
with the new last-use copy elision optimization in an incorrect way.

Fix#81019

…rmation
The last use information is more general in most cases (for example, the
old logic cannot allow the optimization when there are previous field
writes to the implicit byref). In addition the old logic can interact
with the new last-use copy elision optimization in an incorrect way.
Fixdotnet#81019
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 23, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

The last use information is more general in most cases (for example, the
old logic cannot allow the optimization when there are previous field
writes to the implicit byref). In addition the old logic can interact
with the new last-use copy elision optimization in an incorrect way.

Fix #81019

Author:jakobbotsch
Assignees:jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone:-

@jakobbotsch

Copy link
Copy Markdown
MemberAuthor

SuperPMI jobs seemingly did not trigger..?

@runfoapprunfoappBot mentioned this pull request Jan 23, 2023
The zeroing that the tailcall-to-loop optimization does was zeroing the
promoted copies implicit byrefs even when promotion of them was undone.
This was introducing unexpected references to the promoted fields.
Fixdotnet#81081
@jakobbotsch

jakobbotsch commented Jan 24, 2023

Copy link
Copy Markdown
MemberAuthor

This is blocked on #81083 for now.

@jakobbotsch
jakobbotsch marked this pull request as ready for review January 25, 2023 23:33
@jakobbotsch

Copy link
Copy Markdown
MemberAuthor

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs. The new misses are tailcall candidates (that hit a new updateEntryForTailcall) -- these would presumably add some mostly positive mix of diffs.

The regressions are in similar situations as the case inside #81019 -- i.e. the old logic prefers omitting the copy at some non-last use because it believes a subsequent use won't alias. That allows tailcalling, so it is usually better, but the case is quite rare as can be seen in the diffs.

@jakobbotsch

Copy link
Copy Markdown
MemberAuthor

Ping @AndyAyersMS

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

Sorry to see all that fabulous alias analysis get deleted, but this approach is better and more obviously correct.

@jakobbotsch

Copy link
Copy Markdown
MemberAuthor

Sorry to see all that fabulous alias analysis get deleted

Yes, I felt both good and bad removing that code... :-)

@jakobbotsch
jakobbotsch merged commit 75effa9 into dotnet:mainJan 27, 2023
@jakobbotsch
jakobbotsch deleted the last-use-tailcalls branch January 27, 2023 20:52
@jakobbotschjakobbotsch mentioned this pull request Feb 6, 2023
20 tasks
@ghostghost locked as resolved and limited conversation to collaborators Feb 27, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

JIT: Last-use copy elision based on ref counts and liveness interact in incorrect ways

2 participants

@jakobbotsch@AndyAyersMS