bpo-29469: Move constant folding at AST level by methane · Pull Request #2858 · python/cpython

methane · 2017-07-25T07:10:57Z

https://bugs.python.org/issue29469

methane · 2017-07-25T07:38:37Z

I'll check all comments in https://bugs.python.org/review/29469/
Please don't review for a while.

rhettinger · 2017-07-26T06:24:29Z

Overall, this looks great. The code is remarkably clean.

methane · 2017-09-17T16:19:09Z

Python/importlib.h is conflicted very often. So I'll fix it right before merge.

serhiy-storchaka · 2017-10-05T14:02:07Z

See my comment on the tracker: https://bugs.python.org/issue29469#msg297596

serhiy-storchaka · 2017-11-20T11:45:58Z

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.

serhiy-storchaka

It may be worth to document this change in What's New. Even without additional optimizations it can optimize more than the peephole optimizer.

serhiy-storchaka · 2017-12-12T14:55:14Z

Python/ast_opt.c

+    case NameConstant_kind:
+        return e->v.NameConstant.value;
+    default:
+        assert(!is_const(e));


Py_UNREACHABLE() is now used in compile.c.

serhiy-storchaka · 2017-12-12T14:56:44Z

Python/ast_opt.c

+    }
+}
+
+static int make_const(expr_ty node, PyObject *val, PyArena *arena)


Move static int to a separate line. And similarly in other functions below.

serhiy-storchaka · 2017-12-12T15:12:44Z

Python/ast_opt.c

+
+    typedef PyObject *(*unary_op)(PyObject*);
+    static const unary_op ops[] = {
+        0,


AFAIK in C99 you can write

[Invert] = PyNumber_Invert, [Not] = unary_not, ...

serhiy-storchaka · 2017-12-12T15:17:51Z

Python/ast_opt.c

+    Py_ssize_t size = PyObject_Size(newval);
+    if (size == -1) {
+        if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt))
+            return 1;


newval leaked.

serhiy-storchaka · 2017-12-12T15:36:33Z

Python/ast_opt.c

+    }
+
+    /* Avoid creating large constants. */
+    Py_ssize_t size = PyObject_Size(newval);


newval can be NULL.

serhiy-storchaka · 2017-12-12T15:47:46Z

Python/ast_opt.c

+static int make_const(expr_ty node, PyObject *val, PyArena *arena)
+{
+    if (val == NULL) {
+        if(!PyErr_ExceptionMatches(PyExc_KeyboardInterrupt))


Missed space after if. Missed braces.

serhiy-storchaka · 2017-12-12T15:49:13Z

Python/ast_opt.c

+            !is_const(arg) ||
+            /* TODO: handle other types of slices */
+            slice->kind != Index_kind ||
+            !is_const(slice->v.Index.value)) {


Move { to a new line.

serhiy-storchaka · 2017-12-12T16:13:09Z

Python/ast_opt.c

+
+#undef CALL
+#undef CALL_OPT
+#undef CALL_SEQ


#undef CALL_INT_SEQ

serhiy-storchaka · 2017-12-12T16:17:06Z

Python/ast_opt.c

+#include "Python-ast.h"
+
+
+/* TODO: is_const and get_const_value is copied from Python/compile.c.


s/is/are/ ?

serhiy-storchaka · 2017-12-12T16:37:21Z

Lib/test/test_plistlib.py

                    'second': [1, 2],
                    'third': [3, 4],
                })
-                self.assertIsNot(pl2['first'], pl2['second'])


Good catch! This test can fail in other implementations. It should be fixed in 3.6 too. Opened #4813 for this.

serhiy-storchaka · 2017-12-13T10:46:58Z

Python/ast_opt.c

    if (size == -1) {
-        if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt))
-            return 1;
+        Py_DECREF(newval);


Only before return.

serhiy-storchaka

LGTM if fix a one simple error.

serhiy-storchaka · 2017-12-13T18:36:31Z

Generated files not up to date.

serhiy-storchaka · 2017-12-14T08:00:20Z

Great! Many thanks @methane for your work!

Now I'll try to implement additional optimizations.

adrian17 · 2017-12-14T10:49:46Z

Python/ast_opt.c

+    expr_ty arg = node->v.UnaryOp.operand;
+
+    if (!is_const(arg)) {
+        /* Fold not into comparison */


Looks like this makes this peephole pass unnecessary too?

Ah, you're right.

Created #4863. Thanks.

the-knights-who-say-ni added the CLA signed label Jul 25, 2017

methane changed the title ~~bpo-29469: Move constant folding at AST level~~ bpo-29469: Move constant folding at AST level (wip, don't review) Jul 25, 2017

methane force-pushed the ast-constant-fold branch from 93677ba to 86c73de Compare July 26, 2017 02:27

methane changed the title ~~bpo-29469: Move constant folding at AST level (wip, don't review)~~ bpo-29469: Move constant folding at AST level Jul 26, 2017

methane added 3 commits July 26, 2017 16:04

bpo-29469: Move constant folding at AST level

b0c4745

stable visitor order

a4edbc3

remove simple constant folding from peephole

9b0f10c

methane force-pushed the ast-constant-fold branch from a776555 to b0b9db9 Compare July 26, 2017 11:49

stabilize prototype order

a603d5b

methane force-pushed the ast-constant-fold branch from b0b9db9 to a603d5b Compare July 26, 2017 16:35

methane requested review from serhiy-storchaka and vstinner September 17, 2017 16:18

Merge remote-tracking branch 'origin/master' into ast-constant-fold

e2ae3fd

methane requested a review from a team as a code owner October 6, 2017 11:59

serhiy-storchaka removed the request for review from a team October 7, 2017 19:51

methane added 4 commits December 4, 2017 17:56

Merge remote-tracking branch 'origin/master' into ast-constant-fold

43ae08a

Remove code generator for ast_opt.c

04fb56c

remove noop functions

5da0739

make regen-all

af2a46b

methane requested a review from a team December 12, 2017 13:22

Fix test.test_plistlib.TestPlistlib.test_tuple_members

078c1ed

serhiy-storchaka reviewed Dec 12, 2017

View reviewed changes

methane added 3 commits December 13, 2017 14:39

Merge remote-tracking branch 'origin/master' into ast-constant-fold

b64240d

return type should be separated line

89c54ad

Use Py_UNREACHABLE()

94160d4

methane added 5 commits December 13, 2017 15:16

Fix KeyboardInterrupt handling

e416c5f

Use designated initializer

3f34890

Fix error handling in fold_binop()

e1f466d

fix

1f3fc8e

Add NEWS entry

17396dc

serhiy-storchaka reviewed Dec 13, 2017

View reviewed changes

serhiy-storchaka approved these changes Dec 13, 2017

View reviewed changes

bedevere-bot added the awaiting merge label Dec 13, 2017

fix

de2effe

make regen-all

30edda9

methane merged commit 7ea143a into python:master Dec 14, 2017

bedevere-bot removed the awaiting merge label Dec 14, 2017

methane deleted the ast-constant-fold branch December 14, 2017 07:47

adrian17 reviewed Dec 14, 2017

View reviewed changes

s-cork mentioned this pull request Jul 24, 2021

optimized ast skulpt/skulpt_parser#108

Closed

		#include "Python-ast.h"


		/* TODO: is_const and get_const_value is copied from Python/compile.c.

Uh oh!

Conversation

methane commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

methane commented Jul 25, 2017

Uh oh!

rhettinger commented Jul 26, 2017

Uh oh!

methane commented Sep 17, 2017

Uh oh!

serhiy-storchaka commented Oct 5, 2017

Uh oh!

serhiy-storchaka commented Nov 20, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

methane commented Jul 25, 2017 •

edited

Loading

serhiy-storchaka commented Dec 13, 2017 •

edited

Loading