Uh oh!
There was an error while loading. Please reload this page.
gh-132732: Automatically constant evaluate pure operations - #132733
Conversation
Uh oh!
There was an error while loading. Please reload this page.
brandtbucher
left a comment
There was a problem hiding this comment.
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:_PyStackRefleft=PyStackRef_FromPyObjectBorrow(sym_get_const(ctx, stack_pointer[-2]));
_PyStackRefright=PyStackRef_FromPyObjectBorrow(sym_get_const(ctx, stack_pointer[-1]));
_PyStackRefres;
// 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!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Fidget-Spinner
commented
Apr 24, 2025
Seems feasible. I could try to rewrite all occurences of the variable with a stackref-producing const one. Let me try that. |
Fidget-Spinner
commented
Apr 25, 2025
I've verified no refleak on |
markshannon
commented
Apr 30, 2025
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 |
markshannon
commented
Apr 30, 2025
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? |
brandtbucher
commented
Apr 30, 2025
Hm, I think I’d prefer not to. Sounds like it could hurt performance, especially for the JIT (where things can’t inline). |
brandtbucher
commented
Apr 30, 2025
I think a good progression would be:
|
Fidget-Spinner
commented
Apr 30, 2025
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. |
Fidget-Spinner
commented
May 8, 2025
@brandtbucher@markshannon what can I do to get this PR moving? @tomasr8 if youd like to review, here's a summary of the PR:
|
tomasr8
commented
May 8, 2025
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 :) |
tomasr8
left a comment
There was a problem hiding this comment.
Only had time to skim the PR, I'll do a more thorough review this weekend :)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-Authored-By: Tomas R. <tomas.roun8@gmail.com>
markshannon
commented
Jun 20, 2025
In case it wasn't clear. The helper functions would be: |
Fidget-Spinner
commented
Jun 20, 2025
Is this the same as this?
|
markshannon
left a comment
There was a problem hiding this comment.
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.
| } | ||
| 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.
This isn't "pure". It can do anything
| 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.
This is only used in the optimizer emitter (and I don't think it is needed there anyway)
| write_header, | ||
| Emitter, | ||
| TokenIterator, | ||
| skip_to, |
| ) -> bool: | ||
| skip_to(tkn_iter, "SEMI") | ||
| emitter = OptimizerConstantEmitter(self.out, {}, self.original_uop, copy.deepcopy(self.stack)) |
There was a problem hiding this comment.
| emitter=OptimizerConstantEmitter(self.out, {}, self.original_uop, copy.deepcopy(self.stack)) | |
| emitter=OptimizerConstantEmitter(self.out, {}, self.original_uop, self.stack.copy()) |
| """ | ||
| import argparse | ||
| import copy |
There was a problem hiding this comment.
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.
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.
You already have the input names from the uop, [item.name for item in uop.stack.inputs]
There was a problem hiding this comment.
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.
| 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.
| emitter=OptimizerEmitter(out, {}, uop, copy.deepcopy(stack)) | |
| emitter=OptimizerEmitter(out, {}, uop, stack.copy()) |
| return "JitOptRef " | ||
| def stackref_type_name(var: StackItem) -> str: | ||
| if var.is_array(): |
There was a problem hiding this comment.
| ifvar.is_array(): | |
| assertnotvar.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: |
Fidget-Spinner
commented
Jun 27, 2025
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. |
markshannon
left a comment
There was a problem hiding this comment.
That's much better. Thanks.
bedevere-bot
commented
Jun 27, 2025
|
| return (typ == &PyLong_Type) || | ||
| (typ == &PyUnicode_Type) || | ||
| (typ == &PyFloat_Type) || | ||
| (typ == &PyTuple_Type) || |
There was a problem hiding this comment.
@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.
We should also add None here, as well.
…honGH-132733) This adds a "macro" to the optimizer DSL called "REPLACE_OPCODE_IF_EVALUATES_PURE", which allows automatically constant evaluating a bytecode body if certain inputs have no side effects upon evaluations (such as ints, strings, and floats). Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
…honGH-132733) This adds a "macro" to the optimizer DSL called "REPLACE_OPCODE_IF_EVALUATES_PURE", which allows automatically constant evaluating a bytecode body if certain inputs have no side effects upon evaluations (such as ints, strings, and floats). Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
…honGH-132733) This adds a "macro" to the optimizer DSL called "REPLACE_OPCODE_IF_EVALUATES_PURE", which allows automatically constant evaluating a bytecode body if certain inputs have no side effects upon evaluations (such as ints, strings, and floats). Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
…honGH-132733) This adds a "macro" to the optimizer DSL called "REPLACE_OPCODE_IF_EVALUATES_PURE", which allows automatically constant evaluating a bytecode body if certain inputs have no side effects upon evaluations (such as ints, strings, and floats). Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
Uh oh!
There was an error while loading. Please reload this page.