Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-126835: Set location for noped out instructions after constant folding in CFG. #130109

Merged
merged 4 commits into from
Feb 14, 2025

Conversation

WolframAlph
Copy link
Contributor

@WolframAlph WolframAlph commented Feb 14, 2025

@WolframAlph
Copy link
Contributor Author

WolframAlph commented Feb 14, 2025

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

@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
@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
*/
static void
nop_out(basicblock *bb, int start, int count)
nop_out(basicblock *bb, int start, int count, _Py_SourceLocation *location)
Copy link
Member

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.

Copy link
Contributor Author

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!

Copy link
Contributor Author

@WolframAlph WolframAlph Feb 14, 2025

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.

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)

Copy link
Member

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).

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@WolframAlph
Copy link
Contributor Author

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

@@ -1479,40 +1487,16 @@ optimize_lists_and_sets(basicblock *bb, int i, int nextop,
else {
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);
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

@WolframAlph WolframAlph Feb 14, 2025

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.

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);
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, leave it.

@iritkatriel iritkatriel added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 14, 2025
@bedevere-bot
Copy link

🤖 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.

@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
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants