GeezerVison

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

GeezerVison is showing github.com — GH-130415: Improve the JIT's unneeded uop removal pass by brandtbucher · Pull Request #132333 · python/cpython

Conversation

brandtbucher commented Apr 9, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

This improves our ability to remove unused constant and local variable loads during the remove_unneeded_uops pass in the JIT's optimizer.

It now handles all combinations of:

_COPY
_LOAD_CONST_INLINE
_LOAD_CONST_INLINE_BORROW
_LOAD_FAST
_LOAD_FAST_BORROW
_LOAD_SMALL_INT

...followed by...

_POP_TOP
_POP_TOP_LOAD_CONST_INLINE
_POP_TOP_LOAD_CONST_INLINE_BORROW
_POP_TWO_LOAD_CONST_INLINE_BORROW

(It also fixes an issue where CHECK_VALIDITY wasn't being added after these latter four instructions, due to how the old switch-case was structured.)

According to the stats, this results in:

1% reduction in _LOAD_FAST
94% reduction in _POP_TOP_LOAD_CONST_INLINE_BORROW
100% reduction in _POP_TOP_LOAD_CONST_INLINE

@savannahostrowski: We should see even more impact once you start using _POP_TWO_LOAD_CONST_INLINE_BORROW in more places. I'm currently working on turning basically all cached class attribute/method loads into constants too, which should help here as well.

Issue: Better constant narrowing in the JIT optimizer #130415

Sorry, something went wrong.

Uh oh!

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

savannahostrowski reacted with rocket emoji

All reactions

🚀 1 reaction

brandtbucher added 2 commits

April 8, 2025 14:16


          Add more comprehensive peepholing to the JIT

edb68e1


          blurb add

aada255

brandtbucher added performance

Performance or resource usage

interpreter-core

(Objects, Python, Grammar, and Parser dirs)

topic-JIT labels

Apr 9, 2025

brandtbucher self-assigned this

Apr 9, 2025

brandtbucher requested a review from Fidget-Spinner as a code owner

April 9, 2025 21:27

bedevere-app Bot added the awaiting core review label

Apr 9, 2025

bedevere-app Bot mentioned this pull request

Apr 9, 2025

Better constant narrowing in the JIT optimizer #130415

Closed

13 of 17 tasks

Fidget-Spinner approved these changes

Apr 11, 2025

View reviewed changes

Fidget-Spinner left a comment

Copy link

Copy Markdown

Member

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.

I think this should be fine? optimize_pop_top_again should reach a fixpoint (there are only a finite number of last we can change to _NOP).

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-app Bot added awaiting merge and removed awaiting core review labels

Apr 11, 2025

markshannon reviewed

Apr 14, 2025

View reviewed changes

markshannon left a comment

Copy link

Copy Markdown

Member

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.

I have a few questions.
The overall design looks good.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_capi/test_opt.py

		load_attr_bottom = opnames.index("_LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES", call)
		self.assertEqual(opnames[:load_attr_top].count("_GUARD_TYPE_VERSION"), 1)
		self.assertEqual(opnames[call:load_attr_bottom].count("_CHECK_VALIDITY"), 1)
		self.assertEqual(opnames[call:load_attr_bottom].count("_CHECK_VALIDITY"), 2)

markshannon Apr 10, 2025

Copy link

Copy Markdown

Member

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.

Why do we have two _CHECK_VALIDITYs now?

Sorry, something went wrong.

Uh oh!

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

All reactions

brandtbucher Apr 18, 2025

Copy link

Copy Markdown

Member Author

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.

There used to be one validity check, after the setattr call. This adds an additional one after the _POP_TOP following the call, since in theory the function could return an object with a destructor that invalidates the JIT code when it's popped.

Because of the old structure of this optimization pass, we weren't actually treating _POP_TOP as escaping, even though it is (this is handled in the default case, which we fall through to now).

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/optimizer_analysis.c Outdated

		switch (last->opcode) {
		case _POP_TWO_LOAD_CONST_INLINE_BORROW:
		last->opcode = _POP_TOP;
		goto optimize_pop_top_again;

markshannon Apr 14, 2025

Copy link

Copy Markdown

Member

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.

Do we want to optimize again here?
If the previous instruction is _POP_TOP I see no further optimization to apply here.

Sorry, something went wrong.

Uh oh!

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

brandtbucher reacted with thumbs up emoji

All reactions

👍 1 reaction

brandtbucher Apr 18, 2025

Copy link

Copy Markdown

Member Author

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.

Nice catch. We might want to combine smaller ops into larger ones later (for example, turn _POP_TOP; _POP_TOP_LOAD_CONST_INLINE_BORROW into _NOP; _POP_TWO_LOAD_CONST_INLINE_BORROW), which is where I think I was going with this. But that can come later.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/optimizer_analysis.c

		last->opcode = _NOP;
		buffer[pc].opcode = _NOP;
		}
		if (last->opcode == _REPLACE_WITH_TRUE) {

markshannon Apr 14, 2025

Copy link

Copy Markdown

Member

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.

Are we not optimizing this any more? I would have thought it would go in the same case as _POP_TOP_LOAD_CONST_INLINE_BORROW.

Sorry, something went wrong.

Uh oh!

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

All reactions

brandtbucher Apr 18, 2025

Copy link

Copy Markdown

Member Author

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.

Nice catch. Though these should probably just be turned into _POP_TOP_LOAD_CONST_INLINE_BORROW by the optimizer instead.

I'll leave it this way and open up another PR for that. Having the hardcoded constant makes things more difficult, since we'd need to start writing operands instead of operating purely on opcodes.

Sorry, something went wrong.

Uh oh!

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

All reactions

brandtbucher Apr 18, 2025

Copy link

Copy Markdown

Member Author

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.

Actually, I just added that change as part of this PR.

Sorry, something went wrong.

Uh oh!

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

All reactions

brandtbucher added 3 commits

April 18, 2025 14:06


          Remove unnecessary loop.

e0ad739


          Turn _REPLACE_WITH_TRUE into _POP_TOP_LOAD_CONST_INLINE_BORROW

97dbcce


          fixup

9b64c17

brandtbucher merged commit 4f7f72c into python:main

Apr 21, 2025

bedevere-app Bot removed the awaiting merge label

Apr 21, 2025

Reviewers

markshannon

markshannon left review comments

Fidget-Spinner

Fidget-Spinner approved these changes

Uh oh!

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

Assignees

brandtbucher

Labels

interpreter-core

(Objects, Python, Grammar, and Parser dirs)

performance

Performance or resource usage

topic-JIT

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.

3 participants

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.

Conversation

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

Uh oh!

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brandtbucher commented Apr 9, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

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