-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
gh-132732: Automatically constant evaluate pure operations #132733
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
Conversation
Misc/NEWS.d/next/Core_and_Builtins/2025-04-19-16-22-47.gh-issue-132732.jgqhlF.rst
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really neat!
Other than two opcodes I found that shouldn't be marked pure
, I just have one thought:
Rather than rewriting the bodies like this to use the symbols-manipulating functions (which seems error-prone), would we be able to just use stackrefs to do this?
For example, _BINARY_OP_ADD_INT
is defined like this:
PyObject *left_o = PyStackRef_AsPyObjectBorrow(left);
PyObject *right_o = PyStackRef_AsPyObjectBorrow(right);
// ...
res = PyStackRef_FromPyObjectSteal(res_o);
Rather than rewriting uses of these functions, could it be easier to just do something like this, since we're guranteed not to escape?
if (sym_is_const(ctx, stack_pointer[-2]) && sym_is_const(ctx, stack_pointer[-1])) {
// Generated code to turn constant symbols into stackrefs:
_PyStackRef left = PyStackRef_FromPyObjectBorrow(sym_get_const(ctx, stack_pointer[-2]));
_PyStackRef right = PyStackRef_FromPyObjectBorrow(sym_get_const(ctx, stack_pointer[-1]));
_PyStackRef res;
// Now the actual body, same as it appears in executor_cases.c.h:
PyObject *left_o = PyStackRef_AsPyObjectBorrow(left);
PyObject *right_o = PyStackRef_AsPyObjectBorrow(right);
// ...
res = PyStackRef_FromPyObjectSteal(res_o);
// Generated code to turn stackrefs into constant symbols:
stack_pointer[-1] = sym_new_const(ctx, PyStackRef_AsPyObjectSteal(res));
}
I'm not too familiar with the design of the cases generator though, so maybe this is way harder or something. Either way, I'm excited to see this get in!
Seems feasible. I could try to rewrite all occurences of the variable with a stackref-producing const one. Let me try that. |
I've verified no refleak on |
There's a lot going on in this PR, probably too much for one PR. Could we start with a PR to fix up the |
Could we have the default code generator generate a function for the body of the pure instruction and then call that from the three interpreters? |
Hm, I think I’d prefer not to. Sounds like it could hurt performance, especially for the JIT (where things can’t inline). |
I think a good progression would be:
|
I thought about this and I think we can inline if we autogenerate a header file and include that directly. But then we're at the mercy of the compiler in both the normal interpreter and the JIT deciding to inline or not to inline the body again. Which I truly do not want. |
@brandtbucher @markshannon what can I do to get this PR moving? @tomasr8 if youd like to review, here's a summary of the PR:
|
Thanks for the ping! I actually wanted to try/review this PR, I was just very busy this week with work :/ I'll have a look this weekend :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only had time to skim the PR, I'll do a more thorough review this weekend :)
Co-Authored-By: Tomas R. <tomas.roun8@gmail.com>
It should, let me try |
No need to modify I know that doing this all automatically seems purer and more elegant, and it probably is, but the code generator is a bit of a pain point in terms of maintenance. If we can keep it simple with a bit of extra work elsewhere it is usually worth it. One other thing. Ultimately we will want to move this functionality out of the |
In case it wasn't clear. The helper functions would be: |
Is this the same as this?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once skip_to
is removed from generators_common, this becomes nicely self contained.
With it properly encapsulated, it should be easy to modify in the future should we decide to use a slightly different approach.
I've a few more suggestions for tidying things up, but nothing fundamental.
Python/bytecodes.c
Outdated
@@ -829,7 +829,7 @@ dummy_func( | |||
DEOPT_IF(!res); | |||
} | |||
|
|||
pure op(_BINARY_OP_EXTEND, (descr/4, left, right -- res)) { | |||
pure op(_BINARY_OP_EXTEND, (descr/4, left, right -- res)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't "pure". It can do anything
@@ -86,6 +86,16 @@ def emit_to(out: CWriter, tkn_iter: TokenIterator, end: str) -> Token: | |||
out.emit(tkn) | |||
raise analysis_error(f"Expecting {end}. Reached end of file", tkn) | |||
|
|||
def skip_to(tkn_iter: TokenIterator, end: str) -> Token: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only used in the optimizer emitter (and I don't think it is needed there anyway)
) | ||
from generators_common import ( | ||
DEFAULT_INPUT, | ||
ROOT, | ||
write_header, | ||
Emitter, | ||
TokenIterator, | ||
skip_to, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See earlier comment.
) -> bool: | ||
skip_to(tkn_iter, "SEMI") | ||
|
||
emitter = OptimizerConstantEmitter(self.out, {}, self.original_uop, copy.deepcopy(self.stack)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emitter = OptimizerConstantEmitter(self.out, {}, self.original_uop, copy.deepcopy(self.stack)) | |
emitter = OptimizerConstantEmitter(self.out, {}, self.original_uop, self.stack.copy()) |
@@ -4,6 +4,7 @@ | |||
""" | |||
|
|||
import argparse | |||
import copy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stacks already support copying efficiently and correctly.
storage: Storage, | ||
inst: Instruction | None, | ||
) -> bool: | ||
skip_to(tkn_iter, "SEMI") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're just going to throw away the arguments, why have them?
Either check the names match or, better still, don't require the inputs at all.
return not unconditional | ||
|
||
|
||
def replace_opcode_if_evaluates_pure_identifiers(uop: Uop) -> list[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already have the input names from the uop, [item.name for item in uop.stack.inputs]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this is the input names from the REPLACE_OPCODE_IF_EVALUATES_PURE
macro. However, now that you mentioned it, I should get the names from the replacement function, not from this. So thanks for pointing it out.
@@ -175,13 +369,14 @@ def write_uop( | |||
cast = f"uint{cache.size*16}_t" | |||
out.emit(f"{type}{cache.name} = ({cast})this_instr->operand0;\n") | |||
if override: | |||
emitter = OptimizerEmitter(out, {}) | |||
emitter = OptimizerEmitter(out, {}, uop, copy.deepcopy(stack)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emitter = OptimizerEmitter(out, {}, uop, copy.deepcopy(stack)) | |
emitter = OptimizerEmitter(out, {}, uop, stack.copy()) |
@@ -75,6 +80,10 @@ def type_name(var: StackItem) -> str: | |||
return "JitOptRef *" | |||
return "JitOptRef " | |||
|
|||
def stackref_type_name(var: StackItem) -> str: | |||
if var.is_array(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if var.is_array(): | |
assert not var.is_array(), "Unsafe to convert a symbol to an array-like StackRef." |
Check this in the caller if that makes sense, and raise an analysis error there so we aren't relying on assert
to handle input errors.
When you're done making the requested changes, leave the comment: |
Thanks Mark for the review. The code is a lot cleaner now and self-contained. I have made the requested changes; please review again |
Thanks for making the requested changes! @markshannon, @tomasr8: please review the changes made to this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's much better. Thanks.
|
return (typ == &PyLong_Type) || | ||
(typ == &PyUnicode_Type) || | ||
(typ == &PyFloat_Type) || | ||
(typ == &PyTuple_Type) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fidget-Spinner, tuple
isn't safe for comparisons either. The contained items may escape when compared.
We could check the items recursively for safety using sym_tuple_getitem
. Or just remove this.
I'm leaning slightly towards the recursive check, but either could make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also add None
here, as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let's remove it.
Uh oh!
There was an error while loading. Please reload this page.