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-126835: Set location for noped out instructions after constant folding in CFG. by WolframAlph · Pull Request #130109 · python/cpython

Conversation

@WolframAlph

@WolframAlph WolframAlph commented Feb 14, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

cc @Eclips4 @tomasr8

  • Issue: Move const folding to the peephole optimizer #126835

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@WolframAlph
set location for noped out instructions after optimizations
ef68aaa
@WolframAlph
WolframAlph requested review from iritkatriel and markshannon as code owners February 14, 2025 11:17
@bedevere-app bedevere-app Bot added the awaiting review label Feb 14, 2025
@WolframAlph

WolframAlph commented Feb 14, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@iritkatriel I also refactored binop folding to reuse get_constant_sequence logic as we don't need find_load_const_pair anymore.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@WolframAlph WolframAlph changed the title Set location for noped out instructions after optimizations gh-126835: Set location for noped out instructions after optimizations Feb 14, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Feb 14, 2025
Move const folding to the peephole optimizer #126835
Open
@WolframAlph WolframAlph changed the title gh-126835: Set location for noped out instructions after optimizations gh-126835: Set location for noped out instructions after constant folding in CFG. Feb 14, 2025
iritkatriel
iritkatriel reviewed Feb 14, 2025
View reviewed changes
Comment thread Python/flowgraph.c Outdated
*/
static void
nop_out(basicblock *bb, int start, int count)
nop_out(basicblock *bb, int start, int count, _Py_SourceLocation *location)

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

Just set the location to NO_LOCATION. Then the NOP will be discarded.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
WolframAlph reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@WolframAlph WolframAlph Feb 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.

Right, I forgot about NO_LOCATION. Thanks!

Sorry, something went wrong.

Uh oh!

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

@WolframAlph WolframAlph Feb 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.

If we set NO_LOCATION, there is 1 test failing in test_dis.

cpython/Lib/test/test_dis.py

Lines 891 to 912 in 140e69c

dis_loop_test_quickened_code = """\
%3d RESUME_CHECK 0
%3d BUILD_LIST 0
LOAD_CONST_MORTAL 1 ((1, 2, 3))
LIST_EXTEND 1
LOAD_SMALL_INT 3
BINARY_OP 5 (*)
GET_ITER
L1: FOR_ITER_LIST 14 (to L2)
STORE_FAST 0 (i)
%3d LOAD_GLOBAL_MODULE 1 (load_test + NULL)
LOAD_FAST 0 (i)
CALL_PY_GENERAL 1
POP_TOP
JUMP_BACKWARD_{: <6} 16 (to L1)
%3d L2: END_FOR
POP_ITER
LOAD_CONST_IMMORTAL 0 (None)
RETURN_VALUE

There is a bit of a difference in emitted dis, original is (showing only diff part):

887           RESUME_CHECK             0

888           BUILD_LIST               0
              LOAD_CONST_MORTAL        1 ((1, 2, 3))
              LIST_EXTEND              1
              LOAD_SMALL_INT           3
              BINARY_OP                5 (*)
              GET_ITER
      L1:     FOR_ITER_LIST           14 (to L2)
              STORE_FAST               0 (i)

and new dis:

887           RESUME_CHECK             0
              BUILD_LIST               0
              LOAD_CONST_MORTAL        1 ((1, 2, 3))

888           LIST_EXTEND              1
              LOAD_SMALL_INT           3
              BINARY_OP                5 (*)
              GET_ITER
      L1:     FOR_ITER_LIST           14 (to L2)
              STORE_FAST               0 (i)

Sorry, something went wrong.

Uh oh!

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

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

Maybe we need to set the locations on the BUILD_LIST and LOAD_CONST (but don't worry about the NOPs).

Sorry, something went wrong.

Uh oh!

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

@WolframAlph WolframAlph Feb 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.

Yes, it would help. I wonder if that's cleaner than the proposed solution.

Sorry, something went wrong.

Uh oh!

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

@WolframAlph WolframAlph Feb 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.

Anyway, I've updated the PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
WolframAlph added 3 commits February 14, 2025 12:35
@WolframAlph
use NO_LOCATION
0f95636
@WolframAlph
update tests
bdae73b
@WolframAlph
set location for BUILD_SET/BUILD_LIST
cdf427a
@WolframAlph

WolframAlph commented Feb 14, 2025

Copy link
Copy Markdown
Contributor Author

All tests are passing, but we should probably revisit problem of lines after optimizations in CFG in a separate story in future.
All reactions

Sorry, something went wrong.

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
iritkatriel
iritkatriel reviewed Feb 14, 2025
View reviewed changes
Comment thread Python/flowgraph.c
assert(i >= 2);
assert(instr->i_opcode == BUILD_LIST || instr->i_opcode == BUILD_SET);

INSTR_SET_LOC(&bb->b_instr[i-2], instr->i_loc);

@iritkatriel iritkatriel Feb 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 know for sure that the locations of instructions i-1 and i-2 are not already set here?

Sorry, something went wrong.

Uh oh!

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

@WolframAlph WolframAlph Feb 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.

Not sure, but this ensures that new BUILD_LIST/BUILD_SET start from the original line.

Sorry, something went wrong.

Uh oh!

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

@WolframAlph WolframAlph Feb 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.

Chances are we have some bugs with line numbers after optimizations but tests did not catch that. I would say we revisit this in near future. I am not sure we should spend too much time on it now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
iritkatriel
iritkatriel reviewed Feb 14, 2025
View reviewed changes
Comment thread Python/flowgraph.c
Comment on lines +1535 to +1538
PyObject *left = PyTuple_GET_ITEM(pair, 0);
PyObject *right = PyTuple_GET_ITEM(pair, 1);
assert(left != NULL && right != NULL);
PyObject *newconst = PyObject_GetItem(left, right);

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

Suggested change
PyObject *left = PyTuple_GET_ITEM(pair, 0);
PyObject *right = PyTuple_GET_ITEM(pair, 1);
assert(left != NULL && right != NULL);
PyObject *newconst = PyObject_GetItem(left, right);
PyObject *obj = PyTuple_GET_ITEM(pair, 0);
PyObject *attr = PyTuple_GET_ITEM(pair, 1);
assert(obj != NULL && attr != NULL);
PyObject *newconst = PyObject_GetItem(obj, attr);

I this clearer?

Sorry, something went wrong.

Uh oh!

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

@WolframAlph WolframAlph Feb 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.

For subscript yes, but eventually there will be other binary ops as well. I could apply if you want.

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel Feb 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, leave it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Eclips4 Eclips4 added the skip news label Feb 14, 2025
iritkatriel
iritkatriel approved these changes Feb 14, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Feb 14, 2025
@iritkatriel iritkatriel added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 14, 2025
@bedevere-bot

bedevere-bot commented Feb 14, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit cdf427a 🤖

Results will be shown at:

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 14, 2025
@iritkatriel
iritkatriel enabled auto-merge (squash) February 14, 2025 14:15
@iritkatriel
iritkatriel merged commit 334589f into python:main Feb 14, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 14, 2025
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
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

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.

4 participants

@WolframAlph @bedevere-bot @iritkatriel @Eclips4
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.