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-130480: Move duplicate `LOAD_SMALL_INT` optimization from codegen to CFG by WolframAlph · Pull Request #130481 · python/cpython

Conversation

@WolframAlph

@WolframAlph WolframAlph commented Feb 23, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • Issue: Move duplicate LOAD_SMALL_INT optimization from codegen to CFG #130480

cc @Eclips4 @tomasr8

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@WolframAlph
WolframAlph requested review from iritkatriel and markshannon as code owners February 23, 2025 11:59
@bedevere-app bedevere-app Bot added the awaiting review label Feb 23, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Feb 23, 2025
Move duplicate LOAD_SMALL_INT optimization from codegen to CFG #130480
Closed
WolframAlph
WolframAlph commented Feb 23, 2025
View reviewed changes
Comment thread Python/flowgraph.c
if (maybe_instr_make_load_smallint(inst, constant, consts, const_cache)) {
assert(inst->i_opcode == LOAD_SMALL_INT);
}
Py_DECREF(constant);

@WolframAlph WolframAlph Feb 23, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor 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.

For small ints this is effectively no-op as they are immortal, but it helps readability because get_const_value returns new reference.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@WolframAlph
WolframAlph requested a review from ericsnowcurrently as a code owner February 23, 2025 12:25
tomasr8
tomasr8 reviewed Feb 23, 2025
View reviewed changes
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
sobolevn
sobolevn reviewed Feb 23, 2025
View reviewed changes
Comment thread Python/flowgraph.c Resolved

Uh oh!

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

markshannon commented Feb 24, 2025

Copy link
Copy Markdown
Member

Overall, I think this change isn't worthwhile. It doesn't seem to simplify the code at all, but it will slow down the compiler by introducing extra values into the constant array, which then need removing again.
All reactions

Sorry, something went wrong.

Uh oh!

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

WolframAlph commented Feb 24, 2025

Copy link
Copy Markdown
Contributor Author

@markshannon are we ok with having this logic in 2 places then? Also what is your take on #130016?
All reactions

Sorry, something went wrong.

Uh oh!

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

markshannon commented Feb 24, 2025

Copy link
Copy Markdown
Member

What logic in two places?
Codegen emits unoptimized instructions, the cfg optimize optimizes them. I don't see any change to that in this PR.
All reactions

Sorry, something went wrong.

Uh oh!

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

WolframAlph commented Feb 24, 2025

Copy link
Copy Markdown
Contributor Author

@markshannon this:

cpython/Python/flowgraph.c

Lines 1414 to 1422 in 8058390

if (PyLong_CheckExact(newconst)) {
int overflow;
long val = PyLong_AsLongAndOverflow(newconst, &overflow);
if (!overflow && _PY_IS_SMALL_INT(val)) {
assert(_Py_IsImmortal(newconst));
INSTR_SET_OP1(instr, LOAD_SMALL_INT, (int)val);
return SUCCESS;
}
}

&&

cpython/Python/codegen.c

Lines 284 to 291 in 8058390

if (PyLong_CheckExact(o)) {
int overflow;
long val = PyLong_AsLongAndOverflow(o, &overflow);
if (!overflow && _PY_IS_SMALL_INT(val)) {
ADDOP_I(c, loc, LOAD_SMALL_INT, val);
return SUCCESS;
}
}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
WolframAlph added 2 commits March 13, 2025 18:30
@WolframAlph
move LOAD_SMALL_INT optimization from codegen to cfg
222c9f5
@WolframAlph
fix conflicts
f878d78
@WolframAlph
WolframAlph force-pushed the move-smallint branch from 3ecb918 to f878d78 Compare March 13, 2025 17:31
@WolframAlph WolframAlph changed the title gh-130480: Move LOAD_SMALL_INT optimization from codegen to CFG gh-130480: Move duplicate LOAD_SMALL_INT optimization from codegen to CFG Mar 13, 2025
@WolframAlph

WolframAlph commented Mar 13, 2025

Copy link
Copy Markdown
Contributor Author

@iritkatriel can I ask you to review?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iritkatriel
iritkatriel approved these changes Mar 13, 2025
View reviewed changes
Comment thread Python/flowgraph.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_dis.py
Constants:
0: None
0: 1
1: None

@iritkatriel iritkatriel Mar 13, 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 are the constants [1, None]? I thought we always have None in index 0 when there is no docstring.

Sorry, something went wrong.

Uh oh!

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

@WolframAlph WolframAlph Mar 14, 2025

Copy link
Copy Markdown
Contributor 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.

LOAD_CONST 0 (1) is added as the first constant, LOAD_CONST 1 (None) is added as the second constant by _PyCodegen_AddReturnAtEnd. Currently there is 1 slot that is always preserved for constant (in case it is docstring) at index 0. 1 is used constant, so it makes to the final co_consts list, and we have 2 elements in it. Since 1 came first (therefore has lower index), it is first in the final co_consts.

Sorry, something went wrong.

Uh oh!

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

@WolframAlph WolframAlph Mar 14, 2025

Copy link
Copy Markdown
Contributor 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 is #130016 that addresses unused docstring slot which causes confusion.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Mar 13, 2025
@WolframAlph
address review
e3f4f8b
@iritkatriel iritkatriel added skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Mar 14, 2025
iritkatriel
iritkatriel reviewed Mar 14, 2025
View reviewed changes
Comment thread Python/flowgraph.c
{
if (PyLong_CheckExact(newconst)) {
int overflow;
long val = PyLong_AsLongAndOverflow(newconst, &overflow);

@iritkatriel iritkatriel Mar 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.

The return value of PyLong_AsLongAndOverflow needs to be checked for error. Does this mean that maybe_instr_make_load_smallint should also return int, with -1 for error?

Sorry, something went wrong.

Uh oh!

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

@WolframAlph WolframAlph Mar 14, 2025

Copy link
Copy Markdown
Contributor 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.

The return value of PyLong_AsLongAndOverflow needs to be checked for error

I think it was not checked before due to PyLong_CheckExact guard, but I agree we should check it anyway. This is by the way not the only place where this pattern occurs, for example:

cpython/Modules/_cursesmodule.c

Lines 378 to 383 in 10cbd1f

else if (PyLong_CheckExact(obj)) {
int long_overflow;
value = PyLong_AsLongAndOverflow(obj, &long_overflow);
if (long_overflow)
goto overflow;
}

Sorry, something went wrong.

Uh oh!

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

@WolframAlph WolframAlph Mar 14, 2025

Copy link
Copy Markdown
Contributor 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.

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
WolframAlph added 2 commits March 14, 2025 13:27
@WolframAlph
check PyLong_AsLongAndOverflow result for error
fa005f3
@WolframAlph
decref newconst on error
83ccea7
@WolframAlph
WolframAlph requested a review from iritkatriel March 14, 2025 19:07
iritkatriel
iritkatriel reviewed Mar 14, 2025
View reviewed changes
Comment thread Python/flowgraph.c
Comment on lines +2066 to +2068
if (res < 0) {
return ERROR;
}

@iritkatriel iritkatriel Mar 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.

RETURN_IF_ERROR(res);

Sorry, something went wrong.

Uh oh!

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

@WolframAlph WolframAlph Mar 14, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor 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.

maybe_instr_make_load_smallint does not return SUCCESS nor ERROR explicitly, so I am not sure we should do it like this.

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel Mar 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.

ok.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iritkatriel
iritkatriel merged commit 55815a6 into python:main Mar 14, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Mar 14, 2025
plashchynski pushed a commit to plashchynski/cpython that referenced this pull request Mar 17, 2025
@WolframAlph @plashchynski
pythongh-130480: Move duplicate LOAD_SMALL_INT optimization from co…
db6d14e 
…degen to CFG (python#130481)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel approved these changes
@sobolevn sobolevn sobolevn left review comments
@tomasr8 tomasr8 tomasr8 left review comments
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) skip news

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.

5 participants

@WolframAlph @markshannon @iritkatriel @sobolevn @tomasr8
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.