NotificationsYou must be signed in to change notification settings
Fork34k
Star71.3k

GH-139922: Tail calling for MSVC (VS 2026)#143068

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Fidget-Spinner merged 11 commits intopython:mainfromchris-eibl:msvc-tail-call-new

Dec 22, 2025

Merged

GH-139922: Tail calling for MSVC (VS 2026)#143068

Fidget-Spinner merged 11 commits intopython:mainfromchris-eibl:msvc-tail-call-new

Dec 22, 2025

Conversation

Copy link

Member

chris-eibl commentedDec 22, 2025•
edited by bedevere-appbot
Loading

So much has happened on main that I started fresh over and sqash merged the needed things from#139962 onto latest main (mostly because I wanted to use it for my own experiments on main again).

Based on@markshannon's work in#142228 I've factored out_Py_VectorCallInstrumentation_StackRefSteal, because then@Fidget-Spinner's#138115 is not needed in this PR and can be done in a follow-up. An alternative would be to keep it and pasststate into thePy_*_StackRefSteal functions.

Issue:Tail calling interpreter for MSVC #139922

chris-eibl added3 commits

December 21, 2025 14:51

rebased + _Py_VectorCallInstrumentation_StackRefSteal

374874c

Merge commit '374874c3e6d8c4745a52b3bc22044db607b2c9e7' into msvc-tai…

e0cd0f5

…l-call-new

try to fix MSVC tail call CI

7e5555a

bedevere-appbot mentioned this pull request

Dec 22, 2025

Tail calling interpreter for MSVC#139922

Closed

chris-eibl marked this pull request as ready for review

December 22, 2025 14:23

chris-eibl requested review froma team,AA-Turner,ezio-melotti,hugovk andmarkshannon ascode owners

December 22, 2025 14:23

bedevere-appbot added the awaiting review label

Dec 22, 2025

chris-eibl requested a review fromFidget-Spinner

December 22, 2025 14:23

Alternative to restrict and conditional Py_NO_INLINE for PyMapping_Ge…

a75042a

…tOptionalItem

Fidget-Spinner reviewed

Dec 22, 2025

View reviewed changes

Python/bytecodes.cShow resolvedHide resolved

Fidget-Spinner reviewed

Dec 22, 2025

View reviewed changes

Include/abstract.h OutdatedShow resolvedHide resolved

Copy link

Member

Fidget-Spinner commentedDec 22, 2025

In the future, please communicate if you want to supercede a PR for other contributors. It's often considered not good etiquette to take over another person's PR unless they explicitly allow it. However, I'm more than happy to let you take over the PR in this case.

chris-eibl commented

Dec 22, 2025

View reviewed changes

Objects/abstract.c Outdated

		// MSVC fails during a tail call release build with loads of
		// error C4737: Unable to perform required tail call.
		// without using Py_NO_INLINE here, but PGO works fine.
		#if defined(_MSC_VER) && !defined(__clang__) && _Py_TAIL_CALL_INTERP && !defined(_Py_USING_PGO)

Copy link

MemberAuthor

chris-eiblDec 22, 2025

Choose a reason for hiding this comment

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

It needed lots of trial and error to come up with this unintuive minimal change needed to convince MSVC that the local variable passed by reference intoresult here does not escape the tail call.

Using it as an return value inPyMapping_GetOptionalItem2 is more explicit, but might have other drawbacks?

Copy link

MemberAuthor

chris-eibl commentedDec 22, 2025

In the future, please communicate if you want to supercede a PR for other contributors. It's often considered not good etiquette to take over another person's PR unless they explicitly allow it. However, I'm more than happy to let you take over the PR in this case.

Sorry, wasn't ment as superseding, more as a follow up and maybe just a draft for new discussions. Thought about you're gonna take whatever deems appropriate back into your original PR ...

If this here is going to fly, I'd like to include you and Brandt as authors for the commits, but don't know how atm ...

Copy link

Member

hugovk commentedDec 22, 2025

If this here is going to fly, I'd like to include you and Brandt as authors for the commits, but don't know how atm ...

Co-authored-by is the way:

https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

Copy link

Member

Fidget-Spinner commentedDec 22, 2025

If this here is going to fly, I'd like to include you and Brandt as authors for the commits, but don't know how atm ...

Oh I'm happy to be superseded. If you'd like to attribute commit history though, you can just push a trivial commit with the co-author info like Hugo pointed out, and when I squash and merge I'll use that.

make _PyMapping_GetOptionalItem2 private and comment scopes

fdfd8a0

Co-Authored-By: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>Co-Authored-By: Brandt Bucher <brandt@python.org>

Copy link

Member

Fidget-Spinner commentedDec 22, 2025

Try pullingmain in, I fixed a JIT bug there that should turn the JIT CI green again.

Merge with main because aarch64 jit builds started failing

058889e

Copy link

MemberAuthor

chris-eibl commentedDec 22, 2025

Co-authored-by is the way:

Done

hugovk reviewed

Dec 22, 2025

View reviewed changes

.github/workflows/tail-call.ymlShow resolvedHide resolved

.github/workflows/tail-call.yml OutdatedShow resolvedHide resolved

chris-eibl commented

Dec 22, 2025

View reviewed changes

.github/workflows/tail-call.ymlShow resolvedHide resolved

chris-eibland others added2 commits

December 22, 2025 17:44

Apply suggestions from code review

c8d051b

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>

Update .github/workflows/tail-call.yml

ab70524

I could not add this one to batch, so have to do it separatelyCo-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>

Copy link

MemberAuthor

chris-eibl commentedDec 22, 2025

https://github.com/python/cpython/actions/runs/20437258430/job/58722460806 failed inbetween
test_extractall_none_gid (test.test_tarfile.NoneInfoExtractTests_Tar.test_extractall_none_gid) ... Windows fatal exception: access violation
(but got green when rerunning the failed test) even though pulling in main.

I've found this:#143073

Hopefully, this will fix it ...

enable "Emulated Windows Clang (release)"

5c1c240

try to fix "Native Windows MSVC (release)"

Copy link

Member

Fidget-Spinner commentedDec 22, 2025•
edited
Loading

Just ignore the JIT problems for now. The new JIT executor management has a known bug#143026

chris-eibl added2 commits

December 22, 2025 18:26

Merge remote-tracking branch 'upstream/main' into msvc-tail-call-new

a04824f

revert to cmd but keep Hugo's other changes

da1adaf

add --no-progress to choco

Fidget-Spinner added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section label

Dec 22, 2025

Copy link

bedevere-bot commentedDec 22, 2025

🤖 New build scheduled with the buildbot fleet by@Fidget-Spinner for commitda1adaf 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F143068%2Fmerge

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

bedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section label

Dec 22, 2025

Copy link

Member

Fidget-Spinner commentedDec 22, 2025•
edited
Loading

Benchmark results are within noise for the default interpreterhttps://github.com/facebookexperimental/free-threading-benchmarking/blob/main/results/bm-20251222-3.15.0a3%2B-da1adaf/bm-20251222-vultr-x86_64-chris%252deibl-msvc_tail_call_new-3.15.0a3%2B-da1adaf-vs-base.md

Refleak bots are green.

Fidget-Spinner approved these changes

Dec 22, 2025

View reviewed changes

Copy link

Member

Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Once again:

This PR is the successor of one that has been up for 2 months already#139962 . We've had enough discussion there to get a sensing of what we want.
Refleak tests are green, performance results are within noise for the default (computed goto) interpreter.
This PR adds CI for the new configuration, which is also green.
I have discussed this feature with the other core devs a few months back on our bi-weekly perf meetings. Everyone was in agreement that adding scopes to MSVC is fine as it isn't that invasive.
This PR is the minimal changeset required to get TC working on MSVC.
It uses the techniques suggested by Mark in the other PR (just adding scopes and commenting on them). So I expect he should be okay with the overall design I hope. I understand he might disagree on some of the smaller changes.
It LGTM, from my review of the changes.

Therefore, I'm merging this PR.

Any follow-ups can be done on future PRs. Any discussion about the design can be done on the issue itself.