gh-148285: Allow recording uops after specializing uops by adityakrmishra · Pull Request #148373 · python/cpython · GitHub

Fixes Allow recording uops after specializing uops #148285
Replaces closed PR gh-148285: Allow recording uops after specializing uops #148367

The Issue:
Currently, analyzer.py forces any uop with records_value == True to be at index 0. This prevents Tier 2 recording uops from safely trailing Tier 1 specializing uops.

The Fix (V2):
Following feedback from @Sacul0457 on the previous PR, simply checking if the preceding uop was tier == 1 was too permissive (it allowed recording uops to follow any Tier 1 uop, not just specializing ones).

This updated patch uses a strict positional uop_index tracker in add_macro().
A recording uop is now only permitted if:

It is at uop_index == 0 (the very first uop).
OR it is at uop_index == 1 AND the preceding uop's name explicitly starts with _SPECIALIZE_.

CacheEffect (e.g., unused/1) and flush parts do not increment the uop_index, ensuring they remain completely transparent.

This strictly enforces the architecture while allowing structures like:
macro(X) = _SPECIALIZE_X + _RECORD_TOS_TYPE + unused/1 + _X;

Issue: Allow recording uops after specializing uops #148285

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

I think parts[-1] is not equal to the previous UOP: When there is an unused/N between the specialized UOP and the recorded UOP, parts[-1] represents the skip rather than the uop.

Suggested change

      
                # Counts only real OpName entries (not CacheEffect/flush) so we
          
                # know the exact position of each concrete uop inside the macro.
          
                # CacheEffect → becomes Skip; flush → becomes Flush.
          
                # Neither increments uop_index because neither is a "real" uop.
          
                uop_index = 0
          
                prev_uop: Uop | None = None

Suggested change

      
                                if uop.properties.records_value:
          
                                    # A recording uop is legal in exactly two positions:
          
                                    #   1. It is the very first real uop (uop_index == 0).
          
                                    #   2. It is at index 1 AND the immediately preceding
          
                                    #      real uop is a specializing uop, identified by
          
                                    #      the "_SPECIALIZE_" name prefix.
          
                                    #      (Specializing uops are Tier-1-only; recording
          
                                    #      uops are Tier-2-only — they are orthogonal at
          
                                    #      runtime, so this ordering is safe.)
          
                                    preceding_is_specializing = (
          
                                        uop_index == 1
          
                                        and isinstance(parts[-1], Uop)
          
                                        and parts[-1].name.startswith("_SPECIALIZE_")
          
                                    )
          
                                    if uop_index != 0 and not preceding_is_specializing:
          
                                        raise analysis_error(
          
                                            f"Recording uop {part.name} must be first in macro "
          
                                            f"or immediately follow a specializing uop",
          
                                            macro.tokens[0])
          
                                parts.append(uop)
          
                                first = False
          
                                uop_index += 1
          
                                if (uop.properties.records_value 
          
                                    and prev_uop is not None 
          
                                    and "specializing" not in prev_uop.annotations):
          
                                        raise analysis_error(
          
                                            f"Recording uop {part.name} must be first in macro "
          
                                            f"or immediately follow a specializing uop",
          
                                            macro.tokens[0])
          
                                parts.append(uop)
          
                                prev_uop = uop

And we need to add some tests for this pr.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@cocolato Thanks for catching that edge case with the cache skips! I've updated the logic to use a prev_uop tracker as you suggested, which correctly ignores CacheEffect and flush.

I also added unit tests in Tools/cases_generator/test_analyzer.py to cover both valid positions and invalid ones (like a recording uop following a non-specializing Tier 1 uop). Finally, I regenerated the C files locally to ensure the CI check passes. Thanks for the guidance

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@cocolato It looks like regenerating the .c.h files locally on my Windows machine caused some CRLF line-ending or JIT build failures in the CI. I'll leave the C-file regeneration to the automated bots/maintainers from here to avoid messing up the Windows CI runners!

@adityakrmishra there's no need to regen. Also please fix the CLRF stuff on your local machine. You can fix it using pre-commit or git https://stackoverflow.com/questions/2517190/how-do-i-force-git-to-use-lf-instead-of-crlf-under-windows

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@Fidget-Spinner Ah, understood! I've updated my global core.autocrlf setting to prevent Windows from messing with the line endings in the future. I also reverted the two generated C files back to the upstream/main state so they are pristine. Thanks for the tip!

Please remove this file and add any tests to https://github.com/python/cpython/blob/main/Lib/test/test_generated_cases.py

exclude specializing too, so that the check is easier.

@Fidget-Spinner Done! I've moved the tests over to Lib/test/test_generated_cases.py and deleted the standalone test file. I also updated the analyzer.py loop to explicitly exclude both specializing and recording uops from the prev_uop tracker as suggested, which elegantly collapses the check into a simple state validation. Thanks for the review and guidance!

Please check the diff on GitHub. They're broken.

@Fidget-Spinner The diff should be completely clean now! I ran a strict restore to wipe the generated C files and ensured the Python test files are properly using LF line endings. You should only see the logic/test additions now.

I'm really sorry, but this is taking a lot of contributor/maintainer time. It seems the PR still has diffs from other random PRs merged in, even after multiple review comments pointing out how to fix it. The tests also do not seem right, as they are not testing that the recording op is generated.

I'm really sorry again, but I will close this PR to let someone else take it up.

@Fidget-Spinner No problem at all, I completely understand! I'm still learning the ropes of managing Git histories on large projects, and I definitely don't want to hold up the JIT progress with a tangled PR. Thank you so much to you and the rest of the team for your time, patience, and reviews on this! I'm going to step back and look for a simpler 'good first issue' to get my bearings in the CPython workflow. Keep up the great work on 3.15!

adityakrmishra requested a review from markshannon as a code owner April 11, 2026 10:10

bedevere-app Bot added the awaiting review label Apr 11, 2026

bedevere-app Bot mentioned this pull request Apr 11, 2026

Allow recording uops after specializing uops #148285

Closed

adityakrmishra mentioned this pull request Apr 11, 2026

gh-148285: Allow recording uops after specializing uops #148367

Closed

cocolato reviewed Apr 11, 2026

View reviewed changes

cocolato mentioned this pull request Apr 11, 2026

JIT recorder: allow multiple consecutive recording ops per macro op #148378

Closed

adityakrmishra requested review from Fidget-Spinner, savannahostrowski and tomasr8 as code owners April 11, 2026 15:42

adityakrmishra added 2 commits April 11, 2026 21:15

pythongh-148285 : Allow recording uops after specializing uops

ca81806

Apply reviewer suggestions, add tests, and regen cases

440b823

adityakrmishra force-pushed the fix-uop-recording-v2 branch from d35f75f to 440b823 Compare April 11, 2026 15:54

Add missing mypy return type annotation in test

1bbac01

Revert generated C files to upstream state to fix CRLF

d9df27e

Fidget-Spinner added the skip news label Apr 11, 2026

Fidget-Spinner reviewed Apr 12, 2026

View reviewed changes

Move tests to Lib/test and simplify prev_uop tracking per review

eb3ea3f

adityakrmishra mentioned this pull request Apr 12, 2026

JIT: _Py_uop_sym_set_recorded_type may receive a non-type at trace optimization time. #148439

Open

Convert line endings to LF, revert C files, delete old test

ae7d3da

Fidget-Spinner closed this Apr 12, 2026

		@@ -0,0 +1,142 @@
		"""Tests for analyzer.py — specifically the add_macro() recording-uop placement rules.

gh-148285: Allow recording uops after specializing uops by adityakrmishra · Pull Request #148373 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-148285: Allow recording uops after specializing uops - #148373

gh-148285: Allow recording uops after specializing uops#148373adityakrmishra wants to merge 6 commits intopython:mainpython/cpython:mainfrom adityakrmishra:fix-uop-recording-v2adityakrmishra/cpython:fix-uop-recording-v2Copy head branch name to clipboard

Conversation

adityakrmishra commented Apr 11, 2026 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app Bot commented Apr 11, 2026

Uh oh!

cocolato Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

cocolato Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

cocolato Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

cocolato commented Apr 11, 2026

Uh oh!

bedevere-app Bot commented Apr 11, 2026

Uh oh!

adityakrmishra commented Apr 11, 2026

Uh oh!

bedevere-app Bot commented Apr 11, 2026

Uh oh!

bedevere-app Bot commented Apr 11, 2026

Uh oh!

adityakrmishra commented Apr 11, 2026

Uh oh!

Fidget-Spinner commented Apr 11, 2026

Uh oh!

bedevere-app Bot commented Apr 11, 2026

Uh oh!

adityakrmishra commented Apr 11, 2026

Uh oh!

Fidget-Spinner Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

adityakrmishra commented Apr 12, 2026

Uh oh!

Fidget-Spinner commented Apr 12, 2026

Uh oh!

adityakrmishra commented Apr 12, 2026

Uh oh!

Fidget-Spinner commented Apr 12, 2026

Uh oh!

adityakrmishra commented Apr 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gh-148285: Allow recording uops after specializing uops#148373
adityakrmishra wants to merge 6 commits into
python:mainfrom
adityakrmishra:fix-uop-recording-v2

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