Skip to content
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

gh-131798: JIT: Split CALL_TYPE_1 into several uops #132419

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tomasr8
Copy link
Member

@tomasr8 tomasr8 commented Apr 11, 2025

This mostly works now, though the there are now two unused stackref variables in executor_cases.c.h (null and callable) and I'm not sure how to convince the cases generator to remove them. I tried:

  • using DEAD(x) in the guard uops - does not work because the stack variable also appears in the output of the uop. I also can't easily pop them off the stack since neither null or callable is TOS.
  • removing DEAD(x) from _CALL_TYPE_1: not possible, apparently this is needed before I can set res.

I'm out of ideas so if anyone knows how to fix it, I'd love to know :)

sym_set_null(null);
}

op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not entirely sure why I cannot do this here:

Suggested change
op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) {
op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused -- callable, unused, unused)) {

I get SyntaxError: Duplicate name unused, though bytecodes.c allows it 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Weird. Maybe unused isn't a meaningful value here like it is in bytecodes.c (maybe @markshannon knows)?

sym_set_null(null);
}

op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) {
Copy link
Member

Choose a reason for hiding this comment

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

Weird. Maybe unused isn't a meaningful value here like it is in bytecodes.c (maybe @markshannon knows)?

@bedevere-app
Copy link

bedevere-app bot commented Apr 11, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

tomasr8 and others added 3 commits April 11, 2025 22:44
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
tomasr8 and others added 2 commits April 12, 2025 22:54
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Comment on lines +850 to +852
if (sym_has_type(arg)) {
res = sym_new_const(ctx, (PyObject *)sym_get_type(arg));
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there an easy way to test this part? i.e. that the res becomes const? AFAIK there are no other uops/guards that check for instances of type so we can't test it indirectly by checking for removed uops..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants