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.

Sign up for GitHub

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

Jump to bottom

bpo-29469: Move constant folding at AST level #2858

Merged
merged 20 commits into from
Dec 14, 2017
Merged

bpo-29469: Move constant folding at AST level #2858

merged 20 commits into from
Dec 14, 2017

Conversation

methane
Copy link
Member

@methane methane commented Jul 25, 2017 •
edited
Loading

@methane 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
Copy link
Member Author

methane commented Jul 25, 2017

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

@methane methane force-pushed the ast-constant-fold branch from 93677ba to 86c73de Compare July 26, 2017 02:27
@methane 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
@rhettinger
Copy link
Contributor

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

@methane methane force-pushed the ast-constant-fold branch from a776555 to b0b9db9 Compare July 26, 2017 11:49
@methane methane force-pushed the ast-constant-fold branch from b0b9db9 to a603d5b Compare July 26, 2017 16:35
@methane
Copy link
Member Author

methane commented Sep 17, 2017

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

@serhiy-storchaka
Copy link
Member

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

@methane methane requested a review from a team as a code owner October 6, 2017 11:59
@serhiy-storchaka serhiy-storchaka removed the request for review from a team October 7, 2017 19:51
@serhiy-storchaka
Copy link
Member

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.

@methane methane requested a review from a team December 12, 2017 13:22
serhiy-storchaka
serhiy-storchaka reviewed Dec 12, 2017
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

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.

Python/ast_opt.c Outdated
case NameConstant_kind:
return e->v.NameConstant.value;
default:
assert(!is_const(e));
Copy link
Member

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.

Python/ast_opt.c Outdated
}
}

static int make_const(expr_ty node, PyObject *val, PyArena *arena)
Copy link
Member

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.

Python/ast_opt.c Outdated

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

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

Python/ast_opt.c Outdated
Py_ssize_t size = PyObject_Size(newval);
if (size == -1) {
if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt))
return 1;
Copy link
Member

Choose a reason for hiding this comment

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

newval leaked.

Python/ast_opt.c
}

/* Avoid creating large constants. */
Py_ssize_t size = PyObject_Size(newval);
Copy link
Member

Choose a reason for hiding this comment

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

newval can be NULL.

Python/ast_opt.c Outdated
static int make_const(expr_ty node, PyObject *val, PyArena *arena)
{
if (val == NULL) {
if(!PyErr_ExceptionMatches(PyExc_KeyboardInterrupt))
Copy link
Member

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.

Python/ast_opt.c Outdated
!is_const(arg) ||
/* TODO: handle other types of slices */
slice->kind != Index_kind ||
!is_const(slice->v.Index.value)) {
Copy link
Member

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.

Python/ast_opt.c Outdated

#undef CALL
#undef CALL_OPT
#undef CALL_SEQ
Copy link
Member

Choose a reason for hiding this comment

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

#undef CALL_INT_SEQ

Python/ast_opt.c Outdated
#include "Python-ast.h"


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

Choose a reason for hiding this comment

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

s/is/are/ ?

Lib/test/test_plistlib.py Outdated
@@ -320,7 +320,6 @@ def test_tuple_members(self):
'second': [1, 2],
'third': [3, 4],
})
self.assertIsNot(pl2['first'], pl2['second'])
Copy link
Member

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.

serhiy-storchaka
serhiy-storchaka reviewed Dec 13, 2017
View reviewed changes
Python/ast_opt.c Outdated
/* Avoid creating large constants. */
Py_ssize_t size = PyObject_Size(newval);
if (size == -1) {
if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt))
return 1;
Py_DECREF(newval);
Copy link
Member

Choose a reason for hiding this comment

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

Only before return.

serhiy-storchaka
serhiy-storchaka approved these changes Dec 13, 2017
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

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.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Dec 13, 2017 •
edited
Loading

Generated files not up to date.

@methane methane merged commit 7ea143a into python:master Dec 14, 2017
@methane methane deleted the ast-constant-fold branch December 14, 2017 07:47
@serhiy-storchaka
Copy link
Member

Great! Many thanks @methane for your work!

Now I'll try to implement additional optimizations.

adrian17
adrian17 reviewed Dec 14, 2017
View reviewed changes
Python/ast_opt.c
expr_ty arg = node->v.UnaryOp.operand;

if (!is_const(arg)) {
/* Fold not into comparison */
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you're right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #4863. Thanks.

@s-cork s-cork mentioned this pull request Jul 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrian17 adrian17 adrian17 left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@vstinner vstinner Awaiting requested review from vstinner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@methane @rhettinger @serhiy-storchaka @adrian17 @the-knights-who-say-ni @bedevere-bot