Skip to content

Fix tailcall regression with compiled F# - #41206

Merged
jkotas merged 2 commits into
dotnet:masterfrom
jkotas:issue-40864
Aug 25, 2020
Merged

Fix tailcall regression with compiled F##41206
jkotas merged 2 commits into
dotnet:masterfrom
jkotas:issue-40864

Conversation

@jkotas

@jkotasjkotas commented Aug 22, 2020

Copy link
Copy Markdown
Member

Fixes#40864

@Dotnet-GitSync-BotDotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 22, 2020
@jkotasjkotas mentioned this pull request Aug 22, 2020
Closed
@jkotas

jkotas commented Aug 22, 2020

Copy link
Copy Markdown
MemberAuthor

@AndyAyersMS Is it possible to trigger the tailcall stress runs from the CI?

@AndyAyersMS

AndyAyersMS commented Aug 22, 2020

Copy link
Copy Markdown
Member

Yes, it is one of the variations in runtime-coreclr jitstress.

@jakobbotsch

jakobbotsch commented Aug 22, 2020

Copy link
Copy Markdown
Member

Nice! And a negative line diff.

You can take InstantiatingStub1 test from jakobbotsch@b1b668a, it is essentially Andy's example slightly modified and should pass with this change. The InstantiatingStub2 test goes through ldftn so that one will need to be removed.

@jkotas
jkotas marked this pull request as ready for review August 23, 2020 01:05
@erozenfeld

erozenfeld commented Aug 23, 2020

Copy link
Copy Markdown
Contributor

@jkotas Once #41059 is merged, it would be good to rebase this one and run runtime-coreclr jitstress and runtime-coreclr libraries-jitstress on top of that change. #41059 forces all tail calls under tailcallstress to be helper-based calls. I'm just waiting for someone to review a test change in System.Threading.Threads.Tests.ThreadTests.ConstructorTest there. Can you review it?

jakobbotsch
jakobbotsch reviewed Aug 23, 2020
View reviewed changes
Comment threadsrc/coreclr/src/vm/jitinterface.cpp Outdated
jakobbotsch
jakobbotsch reviewed Aug 23, 2020
View reviewed changes
Comment threadsrc/coreclr/src/vm/tailcallhelp.cpp Outdated
@jkotas
jkotasforce-pushed the issue-40864 branch 2 times, most recently from 7c00593 to 3425dc3CompareAugust 24, 2020 00:20
jakobbotsch
jakobbotsch reviewed Aug 24, 2020
View reviewed changes
Comment threadsrc/coreclr/src/vm/tailcallhelp.cpp Outdated
@jkotas

jkotas commented Aug 24, 2020

Copy link
Copy Markdown
MemberAuthor

@dotnet/jit-contrib This is blocked on #41059. Ready for review otherwise.

jakobbotsch
jakobbotsch approved these changes Aug 24, 2020
View reviewed changes

@jakobbotschjakobbotsch 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. Nice to see the optimizations also, that should remove a function call from the hot path.

@erozenfeld

erozenfeld commented Aug 24, 2020

Copy link
Copy Markdown
Contributor

@jkotas#41059 has been merged.

AndyAyersMS
AndyAyersMS approved these changes Aug 25, 2020
View reviewed changes

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

Changes LGTM.

Should we also make updates to the design doc?

@jkotas

jkotas commented Aug 25, 2020

Copy link
Copy Markdown
MemberAuthor

design doc

I have read the design doc and I believe everything in the doc is still accurate. I like that the design doc is focused on the concept and it does not go into explaining every detail.

Let me know if there is a specific fact that you would like to see captured.

jkotasand others added 2 commits August 24, 2020 22:49
@jkotas
Fix tailcall regression with compiled F#
9b73547
This change skips instantiating stubs for direct tailcalls and instead passes the inst argument directly to the target method.
Fixesdotnet#40864
@jakobbotsch@jkotas
Add regression test
3dfee65
jkotas
jkotas commented Aug 25, 2020
View reviewed changes
Comment threadsrc/coreclr/src/vm/tailcallhelp.cpp
pCode->EmitCALL(METHOD__STUBHELPERS__NEXT_CALL_RETURN_ADDRESS, 0, 1);
// All arguments are loaded on the stack, it is safe to disable the GC reporting of ArgBuffer now.
// This is optimization to avoid extending argument lifetime unnecessarily.
// We still need to report the inst argument of shared generic code to prevent it from being unloaded. The inst

@jkotasjkotasAug 25, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

FYI: I have noticed that nothing guarantees that the tailcall target function pointer is kept alive and thus it may be unloaded. It would be a very corner-case situation that is unlikely to be ever hit in a real-world. I have opened #41314 on it.

@erozenfeld

erozenfeld commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

@jkotas I recommend also running runtime-coreclr libraries-jitstress.

@jkotas

jkotas commented Aug 25, 2020

Copy link
Copy Markdown
MemberAuthor

The test failure is #40916. libraries-jitstress and jitstress optional runs passed.

@jkotas
jkotas merged commit e94dc2f into dotnet:masterAug 25, 2020
@jkotas
jkotas deleted the issue-40864 branch August 25, 2020 14:18
@jkotas

jkotas commented Aug 25, 2020

Copy link
Copy Markdown
MemberAuthor

/backport to release/5.0

@github-actions

github-actionsBot commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/223822150

@github-actionsgithub-actionsBot mentioned this pull request Aug 25, 2020
Merged
@jakobbotschjakobbotsch mentioned this pull request Sep 2, 2020
Closed
@ghostghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jakobbotschjakobbotschjakobbotsch approved these changes
@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

No one assigned

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tailcall regression with compiled F#

5 participants

@jkotas@AndyAyersMS@jakobbotsch@erozenfeld@Dotnet-GitSync-Bot