gh-115480: Type and constant propagation for int BINARY_OPs#115478
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Fidget-Spinner requested review from gvanrossum and markshannon as code owners
Fidget-Spinner changed the title Uh oh!
There was an error while loading. Please reload this page.
gvanrossum left a comment
gvanrossum left a comment There was a problem hiding this comment.
So IIUC, the only effect so far is that some guards may be eliminated, right?
I have a feeling that collapsing LOAD a; LOAD b; ADD into LOAD_CONST (a+b) should never overshoot the output buffer, but I know there are some gnarly corner cases, so fine to put that into a new PR.
Fidget-Spinner commented Feb 15, 2024
Yes. |
| goto error; | ||
| } | ||
| res = sym_new_const(ctx, temp); | ||
| // TODO replace opcode with constant propagated one and add tests! |
There was a problem hiding this comment.
In future, please make a new issue for this sort of TODOs.
| if (temp == NULL){ | ||
| goto error; | ||
| } | ||
| res = sym_new_const(ctx, temp); |
| if (temp == NULL){ | ||
| goto error; | ||
| } | ||
| res = sym_new_const(ctx, temp); |
| if (temp == NULL){ | ||
| goto error; | ||
| } | ||
| res = sym_new_const(ctx, temp); |
markshannon commented Feb 15, 2024
I think we should add a res=sym_new_const(ctx, temp); if (res==NULL){goto out_of_space}becomes FAIL_IF_NULL(res=sym_new_const(ctx, temp)); |
| if (is_const(left) && is_const(right)){ | ||
| assert(PyLong_CheckExact(get_const(left))); | ||
| assert(PyLong_CheckExact(get_const(right))); | ||
| PyObject *temp = _PyLong_Add((PyLongObject *)get_const(left), |
There was a problem hiding this comment.
This may leak, if temp is mortal.
Likewise for the other _BINARY_OP_... below.
Symbols hold a strong reference to constants, so there is no leak.
3 participants
Small PR to add type/constant propagation for
_BINARY_OP_ADD/SUBTRACT/MULTIPLY_INT.BINARY_OP#115480