gh-115685: Type/values propagate for TO_BOOL in tier 2 by Fidget-Spinner · Pull Request #115686 · python/cpython

Fidget-Spinner · 2024-02-19T19:34:11Z

Issue: Type/value propagation for TO_BOOL #115685

markshannon

All the res = sym_new_... assignments need to wrapped in OUT_OF_SPACE_IF_NULL

bedevere-app · 2024-02-20T12:41:14Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be poked with soft cushions!

gvanrossum · 2024-02-21T02:26:20Z

Hey @Fidget-Spinner did you make the requested changes? You need to do the dance to make Bedevere happy. :-)

Fidget-Spinner · 2024-02-21T03:26:24Z

I have made the requested changes; please review again.

gvanrossum · 2024-02-21T05:26:50Z

Odd. That test is still failing?!

markshannon

Looks good in general.

There are a few optimizations that have been missed, and beware the side effects of C API calls.

gvanrossum · 2024-02-21T17:14:32Z

 assert(PyLong_CheckExact(get_const(right)));
 PyObject *temp = _PyLong_Add((PyLongObject *)get_const(left),
 (PyLongObject *)get_const(right));


Maybe there should be s get_long_const() helper which returns (PyLongObject *)get_const(x) and also includes the assert(PyLong_CheckExact()) call, to make code like this more compact. (Same for float.)

gvanrossum

One more thing.

gvanrossum · 2024-02-21T17:46:37Z

+ pure op (_POP_TOP_LOAD_CONST_INLINE_BORROW, (ptr/4, pop -- value)) {
+ TIER_TWO_ONLY;
+ DECREF_INPUTS();
+ value = ptr;
+ }


Maybe there could be an even more specialized opcode that doesn't DECREF its input? That would be handy for the case where you introduced this -- the input is known to be None, so we don't really need to bother with the DECREF of the immortal value.

The expectation is that the _POP_TOP part will be removed in a later pass.
LOAD_CONST ; _POP_TOP_LOAD_CONST_INLINE_BORROW -> _LOAD_CONST_INLINE_BORROW

markshannon

Two suggestions

gvanrossum

LGTM, let's see if @markshannon agrees.

Fidget-Spinner · 2024-02-23T07:54:24Z

+{
+ sym_set_type(sym, Py_TYPE(const_val));
+ sym_set_flag(sym, TRUE_CONST | KNOWN | NOT_NULL);
+ Py_XSETREF(sym->const_val, Py_NewRef(const_val));


This is temporary, just ignore this for now. I will remove it in #115817.

markshannon · 2024-02-23T16:36:33Z


 op(_TO_BOOL_NONE, (value -- res)) {
- if (sym_get_const(value) == Py_None) {
+ if (sym_is_const(value) && (sym_get_const(value) == Py_None)) {


Why is this necessary? Shouldn't sym_get_const return NULL for anything that isn't a constant?
It does seem rather verbose to have to prefix every sys_get_const with a sys_is_const.

In the code:
staticinlinePyObject*sym_get_const(_Py_UOpsSymType*sym) { assert(sym_is_const(sym)); assert(sym->const_val); returnsym->const_val; }
So we assume there is always something there when there's sym_get_const. I think I can just change the contract to return NULL on non constants though.

The new sym_get_const() indeed returns NULL in this case.

gvanrossum · 2024-02-28T20:14:35Z

@Fidget-Spinner: It looks like this PR is totally out of date. It adds a new ERROR_IF() macro that should probably be done in a separate cleanup pass. I can do that next (once gh-116062 is merged).

The key part of this PR is to add optimizer specializations for _TO_BOOL etc., but it may be simpler to extract those manually and put them in a fresh PR? Or you could do a massive rebase. :-) But I'd still like the ERROR_IF() cleanup to be separated out.

gvanrossum

LGTM, with one nit (to make merging of my PR simpler).

gvanrossum

Excellent!

gvanrossum · 2024-02-29T20:42:14Z

Since you've addressed Mark's question I don't see a reason to wait for his explicit approval.

…H-115686)

Type/values propagate for TO_BOOL in tier 2
1c6517f

bedevere-appBot added the awaiting core review label Feb 19, 2024

Fidget-Spinner requested a review from markshannon February 19, 2024 19:34

bedevere-appBot mentioned this pull request Feb 19, 2024
Type/value propagation for TO_BOOL#115685
Closed
2 tasks

Fidget-Spinner added the skip news label Feb 19, 2024

Fidget-Spinner mentioned this pull request Feb 20, 2024
gh-115480: Type propagate _BINARY_OP_ADD_UNICODE #115710
Merged

markshannon requested changes Feb 20, 2024
View reviewed changes

Comment threadPython/tier2_redundancy_eliminator_bytecodes.c Outdated
Comment threadPython/tier2_redundancy_eliminator_bytecodes.c Outdated
Comment threadPython/tier2_redundancy_eliminator_bytecodes.c Outdated

bedevere-appBot added awaiting changes and removed awaiting core review labels Feb 20, 2024

Fidget-Spinner added 2 commits February 20, 2024 20:57

Apply suggestions
7dba454

use OUT_OF_SPACE_IF_NULL
8c4d253

markshannon requested changes Feb 21, 2024
View reviewed changes

Fidget-Spinner added 2 commits February 21, 2024 19:08

Address review
6d35c16

Address Mark's review
e909451

Fidget-Spinner requested a review from gvanrossum as a code owner February 21, 2024 11:39

Eclips4 reviewed Feb 21, 2024
View reviewed changes

Comment threadPython/optimizer_analysis.c Outdated

gvanrossum reviewed Feb 21, 2024
View reviewed changes

partially address review
3315ac5

gvanrossum reviewed Feb 21, 2024
View reviewed changes

markshannon reviewed Feb 22, 2024
View reviewed changes

Comment threadPython/bytecodes.c Outdated
Comment threadPython/tier2_redundancy_eliminator_bytecodes.c Outdated

Fidget-Spinner added 2 commits February 22, 2024 19:35

apply suggestions
6c92d35

Merge remote-tracking branch 'upstream/main' into to_bool_tier2
2c9e9d1

gvanrossum approved these changes Feb 23, 2024
View reviewed changes

bedevere-appBot added awaiting merge and removed awaiting changes labels Feb 23, 2024

fix compilation problems
6018440

Fidget-Spinner commented Feb 23, 2024
View reviewed changes

Forgot sym_is_const
7a480e8

markshannon reviewed Feb 23, 2024
View reviewed changes

Fidget-Spinner added 3 commits February 29, 2024 04:48

remove unneded code
5212231

Merge remote-tracking branch 'upstream/main' into to_bool_tier2
f0a7fdf

Merge upstream changes
4c9c97b

gvanrossum approved these changes Feb 28, 2024
View reviewed changes

Comment threadPython/optimizer_bytecodes.c Outdated

gvanrossum reviewed Feb 29, 2024
View reviewed changes

Comment threadPython/optimizer_bytecodes.c Outdated

Fidget-Spinner added 3 commits March 1, 2024 03:04

Merge remote-tracking branch 'upstream/main' into to_bool_tier2
7025620

check for bottom
6ddc97c

add space
cdd15bd

gvanrossum approved these changes Feb 29, 2024
View reviewed changes

gvanrossum requested a review from markshannon February 29, 2024 20:41

Fidget-Spinner merged commit d01886c into python:mainFeb 29, 2024

bedevere-appBot removed the awaiting merge label Feb 29, 2024

Fidget-Spinner deleted the to_bool_tier2 branch February 29, 2024 22:13

gvanrossum mentioned this pull request Feb 29, 2024
Constant propagation for _TO_BOOL #115759
Closed

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
pythongh-115685: Type/values propagate for TO_BOOL in tier 2 (pythonG…
9bb38c5
…H-115686)

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
pythongh-115685: Type/values propagate for TO_BOOL in tier 2 (pythonG…
9eb7c8f
…H-115686)

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
pythongh-115685: Type/values propagate for TO_BOOL in tier 2 (pythonG…
ce60550
…H-115686)

Uh oh!

Uh oh!

Conversation

Fidget-Spinner commented Feb 19, 2024• edited by bedevere-app BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-appBot commented Feb 20, 2024

Uh oh!

gvanrossum commented Feb 21, 2024

Uh oh!

Fidget-Spinner commented Feb 21, 2024

Uh oh!

gvanrossum commented Feb 21, 2024

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gvanrossumFeb 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossumFeb 21, 2024

Choose a reason for hiding this comment

Uh oh!

markshannonFeb 22, 2024

Choose a reason for hiding this comment

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Fidget-SpinnerFeb 23, 2024

Choose a reason for hiding this comment

Uh oh!

markshannonFeb 23, 2024

Choose a reason for hiding this comment

Uh oh!

Fidget-SpinnerFeb 23, 2024

Choose a reason for hiding this comment

Uh oh!

gvanrossumFeb 28, 2024

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Feb 28, 2024

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Feb 29, 2024

Uh oh!

Reviewers

Fidget-Spinner commented Feb 19, 2024•
edited by bedevere-app Bot
Loading