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.

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-128706: Evaluate constant comparisons in fold_compare #128705

Closed
wants to merge 9 commits into from

Conversation

WolframAlph
Copy link
Contributor

@WolframAlph WolframAlph commented Jan 10, 2025

@WolframAlph WolframAlph marked this pull request as draft January 10, 2025 12:23
@WolframAlph WolframAlph changed the title Draft: Evaluate constant comparions in fold_compare gh-128706: Evaluate constant comparions in fold_compare Jan 10, 2025
@vstinner
Copy link
Member

Please fix failing tests and add a few more tests in test_ast :-)

@WolframAlph
Copy link
Contributor Author

Hi @vstinner , I am not sure this is worth it since @Eclips4 mentioned that this cannot be done just yet. For the same reason (potential rejection) I did not add new tests and made this PR as a draft.

@tomasr8 tomasr8 changed the title gh-128706: Evaluate constant comparions in fold_compare gh-128706: Evaluate constant comparisons in fold_compare Jan 10, 2025
@vstinner
Copy link
Member

You can remove the "is" optimization for now for example.

@WolframAlph
Copy link
Contributor Author

Okay, so as @vstinner suggested, I removed folding of is/is not ops which left only test_ast.py failing.
I adjusted 2 tests in test_ast.py to be aware of new comparison folding behavior.
Added test suite for constant comparison folding.
Unmarking this PR as Draft now.

@WolframAlph WolframAlph marked this pull request as ready for review January 10, 2025 17:47
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I'm sending this review although I'm not really sure we should do constant comparison folding for now (even though it could help in constant propagation, I think we should address this when the time comes)

Python/ast_opt.c Outdated Show resolved Hide resolved
Python/ast_opt.c Outdated Show resolved Hide resolved
Python/ast_opt.c Show resolved Hide resolved
Python/ast_opt.c Outdated Show resolved Hide resolved
Python/ast_opt.c Outdated Show resolved Hide resolved
Python/ast_opt.c Outdated Show resolved Hide resolved
Python/ast_opt.c Outdated Show resolved Hide resolved
Python/ast_opt.c Outdated Show resolved Hide resolved
Python/ast_opt.c Outdated Show resolved Hide resolved
Python/ast_opt.c Outdated Show resolved Hide resolved
@WolframAlph
Copy link
Contributor Author

@picnixz I guess I addressed all of your comments. Also refactored switch a bit to factor out common logic and added Py_UNREACHABLE for default case in switch just in case.

Python/ast_opt.c Outdated Show resolved Hide resolved
Python/ast_opt.c Outdated Show resolved Hide resolved
Python/ast_opt.c Outdated Show resolved Hide resolved
Python/ast_opt.c Outdated Show resolved Hide resolved
Python/ast_opt.c Outdated Show resolved Hide resolved
@WolframAlph WolframAlph requested a review from picnixz January 11, 2025 13:41
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

The addition seems small enough but I prefer that an AST expert confirms whether this addition is truly needed or not. ISTM that it could be used for constant propogation but depending on how we implement the latter, maybe we should only do this at that time.

@@ -210,6 +210,10 @@ configuration mechanisms).

Other language changes
======================
* Constant comparsion expressions are now folded and evaluated before runtime.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a terminology that would be like "at compilation time" instead of "before runtime"? (using "compilation" time may cause users to think that it's only when .pyc files are created but this can be skipped with -B I think?)

@@ -210,6 +210,10 @@ configuration mechanisms).

Other language changes
======================
* Constant comparsion expressions are now folded and evaluated before runtime.
Copy link
Member

Choose a reason for hiding this comment

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

I don't know how we are actually ordering the changes. Since this is a fairly niche optimization, I would put it at the end of the list.

Copy link
Member

Choose a reason for hiding this comment

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

Usually, new changes are added at the end of a section.

@WolframAlph
Copy link
Contributor Author

As I already mentioned I would expect programming language to make such optimizations regardless how practical or impractical they are. But this my personal opinion and everyone if free to disagree. As I also mentioned this becomes more practical when/if constant propagation is implemented, but as @picnixz mentioned probably it should be addressed when the time comes. I would ask participants/AST experts to come to an agreement. I am under no circumstances pushing this, but I would like to know if I should spend time addressing comments in the code if ultimately this will be rejected. cc @Eclips4

}
int op = asdl_seq_GET(ops, i);
if (op == Is || op == IsNot) {
/* Do not fold "is" and "is not" expressions since this breaks
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* Do not fold "is" and "is not" expressions since this breaks
/* Do not fold "is" and "is not" expressions since it would break

@@ -210,6 +210,10 @@ configuration mechanisms).

Other language changes
======================
* Constant comparsion expressions are now folded and evaluated before runtime.
For example, expressions like: ``"str" in ("str",)`` or ``1 == 1.0 == True``
are now pre-evaluated.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
are now pre-evaluated.
are now pre-evaluated during compilation.

@@ -210,6 +210,10 @@ configuration mechanisms).

Other language changes
======================
* Constant comparsion expressions are now folded and evaluated before runtime.
Copy link
Member

Choose a reason for hiding this comment

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

Usually, new changes are added at the end of a section.

@erlend-aasland
Copy link
Contributor

Closing, as the linked issue has been closed as "wont implement". Thanks for your interest in improving CPython; unfortunately this feature has been rejected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants