bpo-29469: Move constant folding at AST level by methane · Pull Request #2858 · 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
Conversation24 Commits20 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-29469: Move constant folding at AST level bpo-29469: Move constant folding at AST level (wip, don't review)
bpo-29469: Move constant folding at AST level (wip, don't review) bpo-29469: Move constant folding at AST level
Overall, this looks great. The code is remarkably clean.
Python/importlib.h is conflicted very often. So I'll fix it right before merge.
Could you please remove asdl_ct.py and simplify ast_opt.c by getting rid of useless functions like astfold_boolop? The AST types are rarely changed, and I think it would be not hard to update the cleaner code. If sometimes we will need the general code generator we could restore it from your patches.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth to document this change in What's New. Even without additional optimizations it can optimize more than the peephole optimizer.
| case NameConstant_kind: |
|---|
| return e->v.NameConstant.value; |
| default: |
| assert(!is_const(e)); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Py_UNREACHABLE() is now used in compile.c.
| } |
|---|
| } |
| static int make_const(expr_ty node, PyObject *val, PyArena *arena) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move static int to a separate line. And similarly in other functions below.
| typedef PyObject *(*unary_op)(PyObject*); |
| static const unary_op ops[] = { |
| 0, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK in C99 you can write
[Invert] = PyNumber_Invert, [Not] = unary_not, ...
| Py_ssize_t size = PyObject_Size(newval); |
|---|
| if (size == -1) { |
| if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt)) |
| return 1; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newval leaked.
| } |
|---|
| /* Avoid creating large constants. */ |
| Py_ssize_t size = PyObject_Size(newval); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newval can be NULL.
| static int make_const(expr_ty node, PyObject *val, PyArena *arena) |
|---|
| { |
| if (val == NULL) { |
| if(!PyErr_ExceptionMatches(PyExc_KeyboardInterrupt)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed space after if. Missed braces.
| !is_const(arg) || | | ------------------------------------------ | | /* TODO: handle other types of slices */ | | slice->kind != Index_kind || | | !is_const(slice->v.Index.value)) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move { to a new line.
| #undef CALL |
| #undef CALL_OPT |
| #undef CALL_SEQ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #include "Python-ast.h" |
|---|
| /* TODO: is_const and get_const_value is copied from Python/compile.c. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/is/are/ ?
| @@ -320,7 +320,6 @@ def test_tuple_members(self): |
|---|
| 'second': [1, 2], |
| 'third': [3, 4], |
| }) |
| self.assertIsNot(pl2['first'], pl2['second']) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! This test can fail in other implementations. It should be fixed in 3.6 too. Opened #4813 for this.
| /* Avoid creating large constants. */ |
|---|
| Py_ssize_t size = PyObject_Size(newval); |
| if (size == -1) { |
| if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt)) |
| return 1; |
| Py_DECREF(newval); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only before return.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if fix a one simple error.
Generated files not up to date.
Great! Many thanks @methane for your work!
Now I'll try to implement additional optimizations.
| expr_ty arg = node->v.UnaryOp.operand; |
|---|
| if (!is_const(arg)) { |
| /* Fold not into comparison */ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this makes this peephole pass unnecessary too?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #4863. Thanks.