GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-131798: Split `FOR_ITER_GEN` into smaller uops by Sacul0457 · Pull Request #148372 · python/cpython

Conversation

@Sacul0457

@Sacul0457 Sacul0457 commented Apr 11, 2026 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I'm not sure how the optimizer knows the symbolic type on the first guard but it seems to work?

  • Issue: Better uop coverage in the JIT optimizer #131798

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Sacul0457 added 3 commits April 11, 2026 17:58
@Sacul0457
_GUARD_FOR_ITER_GEN_FRAME to _FOR_ITER_GEN_FRAME
0d031c0
@Sacul0457
remove test file, oops
9e44704
@Sacul0457
use unused instead
f588c16
@Sacul0457
Sacul0457 requested review from Fidget-Spinner, markshannon, savannahostrowski and tomasr8 as code owners April 11, 2026 10:06
@bedevere-app bedevere-app Bot added the awaiting review label Apr 11, 2026
@bedevere-app bedevere-app Bot mentioned this pull request Apr 11, 2026
Better uop coverage in the JIT optimizer #131798
Open
@Fidget-Spinner

Fidget-Spinner commented Apr 11, 2026

Copy link
Copy Markdown
Member

I'm not sure how the optimizer knows the symbolic type on the first guard but it seems to work?

Uh oh. you just caught a bug in the JIT optimizer. Would you like to fix it?

The problem is that this line is wrong https://github.com/python/cpython/blob/main/Python/optimizer_symbols.c#L807 it should not be returning PyGen_Type, but rather NULL. Instead, _Py_uop_sym_get_probable_type should be the one returning PyGen_Type.

If you want to fix it, just move the lines around amend this small test to !, and copy the same line and change it to probable type https://github.com/python/cpython/blob/main/Python/optimizer_symbols.c#L2214

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Sacul0457

Sacul0457 commented Apr 11, 2026

Copy link
Copy Markdown
Contributor Author

Sure!
Should I fix it in this PR or create a new issue and open a separate PR?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Apr 11, 2026

Copy link
Copy Markdown
Member

Please open an issue and fix it in that PR. Thank you.

Sacul0457 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Apr 11, 2026
JIT: _Py_uop_sym_get_type returns PyGen_Type instead of NULL #148374
Closed
gh-148374: Fix a bug in _Py_uop_sym_get_type #148375
Merged
@Fidget-Spinner

Fidget-Spinner commented Apr 11, 2026

Copy link
Copy Markdown
Member

@Sacul0457 this might not be worth it, but splitting up _CHECK_AND_ALLOCATE_OBJECT into the CHECK and _ALLOCATE should be worth it (we can remove the redundant checks through same thing as _GUARD_TYPE_VERSION). I just merged a PR into main that should make this easier. Would you like to work on that instead?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Sacul0457

Sacul0457 commented Apr 11, 2026

Copy link
Copy Markdown
Contributor Author

Sure :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Sacul0457 Sacul0457 closed this Apr 11, 2026
@Sacul0457

Sacul0457 commented Apr 11, 2026

Copy link
Copy Markdown
Contributor Author

Hi @Fidget-Spinner!
Does this look right?
For _CHECK_OBJECT it should look something like this

op(_CHECK_OBJECT, (type_version/2, callable, self_or_null, unused[oparg] -- callable, self_or_null, unused[oparg])) {
    PyObject *callable_o = PyStackRef_AsPyObjectBorrow(callable);
    EXIT_IF(!PyStackRef_IsNull(self_or_null));
    // ...
    // ...
    EXIT_IF(FT_ATOMIC_LOAD_UINT32_RELAXED(tp->tp_version_tag) != type_version);
}

Then _ALLOCATE_OBJECT would be everything else.

Then in optimizer_bytecodes.c:

op(_CHECK_OBJECT, (type_version/2, callable, self_or_null, unused[oparg] -- callable, self_or_null, unused[oparg])) {
    PyObject *probable_callable = sym_get_probable_value(callable);
    PyTypeObject *tp = (PyTypeObject *)probable_callable;
    if (tp->tp_version_tag == type_version && sym_is_not_null(self_or_null)) {
        // If the type version has not changed since we last saw it,
        // then we know this __init__ is definitely the same one as in the cache.
        ADD_OP(_NOP, 0, 0);
    }
    else {
        // do what _GUARD_TYPE_VERSION did in its else case
        sym_set_non_null(ctx, callable);
        sym_set_non_null(ctx, self_or_null);
    }
}
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Apr 11, 2026

Copy link
Copy Markdown
Member

@Sacul0457 you need to do similar to what we're currently doing in _GUARD_TYPE_VERSION, which is to set the type watcher if it doesn't match, and leave the first type check around.

The refactored uop looks correct assuming you copied everything in correctly. Please open a PR.

Sacul0457 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Sacul0457
Sacul0457 deleted the Optimize-_FOR_ITER_GEN_FRAME branch April 11, 2026 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@tomasr8 tomasr8 Awaiting requested review from tomasr8 tomasr8 is a code owner
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner Fidget-Spinner is a code owner
@savannahostrowski savannahostrowski Awaiting requested review from savannahostrowski

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@Sacul0457 @Fidget-Spinner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.