-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
Conversation
@iritkatriel I also refactored binop folding to reuse |
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) |
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.
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!
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.
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)
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).
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.
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.
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); |
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?
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.
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.
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); |
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.
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?
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.
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.
🤖 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. |
cc @Eclips4 @tomasr8