Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

GH-135904: Add tests for the JIT build process#136766

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

Open
brandtbucher wants to merge13 commits intopython:main
base:main
Choose a base branch
Loading
frombrandtbucher:jit-tests

Conversation

@brandtbucher
Copy link
Member

@brandtbucherbrandtbucher commentedJul 18, 2025•
edited by bedevere-appbot
Loading

This is overdue, but it's especially valuable now that we're doing more subtle transformations on the machine code at JIT build time. This has a number of benefits:

  • When we add a new optimization at this level, we can directly see the impact on example code in the PR diff. And if it doesn't show up, we can add a test for it.
  • It gives us visibility into the quality of the generated code (without having the full stencils checked in yet). For example, making this PR helped me realize that there are three obvious quality issues on platforms I don't look at much:
    • On some platforms, system headers were putting unused writable data in our read-only data stencils. This is both wrong and wasteful. I fixed this issue as part of this PR (we just optimistically remove this data, and raise if it's actually used).
    • The JIT still inserts frame pointer pushes/pops on Intel-based Macs. I'll fix this in a follow-up PR.
    • LLVM has a bug that inserts unnecessary spills and reloads of C local variables on 32-bit Windows at the beginning and end of every stencil. I've filed an issue upstream:musttail doesn't optimize sibling calls llvm/llvm-project#147813

The new test is inLib/test/test_jit_stencils.py. This test usesTools/jit/test/test_executor_cases.c.h to generate a few small test stencils, and compares them to the expected output in the correspondingTools/jit/test/test_jit_stencils-*.h file.

Most of the changes inTools/jit/_targets.py are just cleanup to make the generated stencils simpler and more consistent, or to fix the writable-data bug I mentioned above.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@brandtbucher for commit867a686 🤖

Results will be shown at:

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

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelJul 18, 2025
@brandtbucher
Copy link
MemberAuthor

!buildbot aarch64 RHEL8 LTO

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@brandtbucher for commit7c9c3ff 🤖

Results will be shown at:

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

The command will test the builders whose names match following regular expression:aarch64 RHEL8 LTO

The builders matched are:

  • aarch64 RHEL8 LTO PR
  • aarch64 RHEL8 LTO + PGO PR

@tomasr8tomasr8 self-requested a reviewJuly 21, 2025 19:56
classTestJITStencils(unittest.TestCase):

def_build_jit_stencils(self,target:str)->str:
withtempfile.TemporaryDirectory()aswork:
Copy link
Member

Choose a reason for hiding this comment

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

I thinkos_helper.temp_dir is a bit more common in tests but either is fine :)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Hm, I'll take a look!

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Is it, like, better? Haha.

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't tell you 😅 but it seems quite common. Though both are used so it probably doesn't matter :)

parser.add_argument(
"-i",
"--input-file",
help="where to find the generated executor cases",

Choose a reason for hiding this comment

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

Do you think it's worth calling out that this is used to pass in the dummy file in our tests and/or that this could be useful for custom builds/debugging? Maybe not in help text but in a comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I left a similar comment at the top of the PR. Is this the case of regenerating the file while in development so then be able to pass it?

savannahostrowski reacted with thumbs up emoji
Copy link
Contributor

@diegorussodiegorusso left a comment

Choose a reason for hiding this comment

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

This is not fully related to this PR but sometime I change this linehttps://github.com/python/cpython/pull/136766/files#diff-700c6057d24addce74e8787edb5f5556f718299f6ef1742cbb1d79580cdb535cR199 and adddelete=False because I want to inspect the temporary files that have been generated.
Do you think that we should expose this to the user (it would be useful for me)?

pyconfig_h=pathlib.Path(sysconfig.get_config_h_filename()).resolve()
result,args=test.support.script_helper.run_python_until_end(
_TOOLS_JIT_BUILD_PY,
"--input-file",_TOOLS_JIT_TEST_TEST_EXECUTOR_CASES_C_H,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just for my understanding. is this needed in case we need to regenerate this during development and we need to test it, right?

HoleValue.JUMP_TARGET:"state->instruction_starts[instruction->jump_target]",
HoleValue.ERROR_TARGET:"state->instruction_starts[instruction->error_target]",
# These should all have raised an error if they were actually used:
#HoleValue.WRITABLE: "",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I fully understand this. Can you expland please?

parser.add_argument(
"-i",
"--input-file",
help="where to find the generated executor cases",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I left a similar comment at the top of the PR. Is this the case of regenerating the file while in development so then be able to pass it?

savannahostrowski reacted with thumbs up emoji
@diegorusso
Copy link
Contributor

Most of the changes in Tools/jit/_targets.py are just cleanup to make the generated stencils simpler and more consistent, or to fix the writable-data bug I mentioned above.

It's not a hard push back, but shouldn't be these changes go to a separate PR?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@diegorussodiegorussodiegorusso left review comments

@tomasr8tomasr8tomasr8 left review comments

@savannahostrowskisavannahostrowskisavannahostrowski left review comments

Assignees

@brandtbucherbrandtbucher

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@brandtbucher@bedevere-bot@diegorusso@tomasr8@savannahostrowski
[8]ページ先頭
©2009-2025 Movatter.jp