-
-
Notifications
You must be signed in to change notification settings - Fork 34k
gh-144549: Fix tail calling interpreter on Windows for FT #144550
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
Conversation
savannahostrowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I do have more meta comments about existing patterns in the workflow file though:
- I'm wondering if
x86_64-pc-windows-msvc/msvc-free-threadingwould be more consistent with the triple/compiler pattern. - It might be a bit more robust to use a matrix variable here like
free-threading: true? If we add more FT variants later, this could get a bit hairy.
Not a problem to solve in this PR since both of these are leveraging existing patterns but maybe some futureproofing we should consider.
Thanks for the review. Yeah I agree we need to refactor the CI files. Unfortunately, I'm too bad at this to refactor the file. Even editing the current one to make it work was already a struggle for me. Let's do this in a follow-up PR or let a new contributor take it. |
|
Yep - agreed. Opened #144552. I'll work on this as a follow up. |
I just moved the body of _LOAD_ATTR into an external function.
The problem was that we had a local variable that has its address taken and "escaped" in the tail calling function. By moving this to an external function, the tail calling function no longer has escaping variables.
Also benefits #141794 due to smaller JIT stencil sizes.