bpo-41323: Perform 'peephole' optimizations directly on the CFG. by markshannon · Pull Request #21517 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation18 Commits8 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
bpo-41323: Move 'peephole' optimizations directly on the CFG. bpo-41323: Perform 'peephole' optimizations directly on the CFG.
| PyObject *names = NULL; |
|---|
| PyObject *varnames = NULL; |
| PyObject *name = NULL; |
| PyObject *freevars = NULL; |
| PyObject *cellvars = NULL; |
| PyObject *bytecode = NULL; |
| // PyObject *bytecode = NULL; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget this?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks.
Mostly, this looks pretty good. Consider removing the peephole.c file altogether. It was originally part of compile.c and got separated out when it got too large. We can more the PyCode_Optimize() function back to compile.c. Ideally, we could drop it from the public api as part of Victor's PEP to overhaul the C API.
One small loss is that the current code has macros that provide meaningful opcode groupings like UNCONDITIONAL_JUMP, CONDITIONAL_JUMP, ABSOLUTE_JUMP, and JUMPS_ON_TRUE. I always found those grouping helpful for reasoning about the code.
| Py_XINCREF(code); |
|---|
| PyMem_Free(blocks); |
| PyMem_Free(codestr); |
| Py_INCREF(code); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use Py_UNUSED for the ones that are not code in the function declaration.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 6b7019a 🤖
If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.
Comment on lines 5885 to 5886
| if (!names || !varnames) | | ------------------------- | | goto error; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (!names || !varnames) | | --------------------------- | | goto error; | | if (!names || !varnames) { | | goto error; | | } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| optimize_cfg(struct assembler *a, PyObject *consts) |
|---|
| { |
| for (int i = 0; i < a->a_nblocks; i++) { |
| if (optimize_basic_block(a->a_reverse_postorder[i], consts)) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this converge on one single iteration? What happens if you have a sequence of many jumps all together between blocks?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Some jumps that can be eliminated will still remain.
This PR is aimed at closely mimicking the behavior of the existing peephole optimizer.
We can improve it later.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great and very compact! This is a very good work @markshannon ! I will do a more thorough review over the weekend but I have left some minor comments for now
| @@ -3645,6 +3646,11 @@ compiler_boolop(struct compiler *c, expr_ty e) |
|---|
| for (i = 0; i < n; ++i) { |
| VISIT(c, expr, (expr_ty)asdl_seq_GET(s, i)); |
| ADDOP_JABS(c, jumpi, end); |
| basicblock *next = compiler_new_block(c); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? Why we need a new blocks here for connecting the jumps?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All instructions following a jump or branch must start a new basic block.
We now need the CFG generated by the code generation pass to be correct, as we perform jump elimination on it, rather than the CFG recreated in peephole.c.
Longer term, I think the front end should use labels and CFG creation should be a pass in between the code generation pass and the optimizer.
@rhettinger
I've removed peephole.c as you suggested.
The UNCONDITIONAL_JUMP, CONDITIONAL_JUMP, ABSOLUTE_JUMP, and JUMPS_ON_TRUE don't seem that useful in an instruction based optimizer.
I initially created versions of them, but it turned out they were unused. So I deleted them.
For jump-to-jump elimination, it turns out that the exact opcode is more important that whether a jump is conditional and/or relative.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need one of these?
| } |
|---|
| } |
| PyObject_Free(stack); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks.
🤖 New build scheduled with the buildbot fleet by @pablogsal for commit fda0bed 🤖
If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.
One final tweak. Jumps to empty blocks that were immediately followed by jumps were not being eliminated. The final commit fixes that.
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request
- Move 'peephole' optimizations into compile.c and perform them directly on the CFG.
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request
- Move 'peephole' optimizations into compile.c and perform them directly on the CFG.
xzy3 pushed a commit to xzy3/cpython that referenced this pull request
- Move 'peephole' optimizations into compile.c and perform them directly on the CFG.