| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
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:
_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!
Sorry, something went wrong.
Seems feasible. I could try to rewrite all occurences of the variable with a stackref-producing const one. Let me try that. |
Sorry, something went wrong.
|
I've verified no refleak on test_capi.test_opt locally apart from #132731 which is pre-existing. |
Sorry, something went wrong.
|
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 pure annotations so that they are on the correct instructions and maybe add the pure_guard annotation that Brandt suggested? |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
Hm, I think I’d prefer not to. Sounds like it could hurt performance, especially for the JIT (where things can’t inline). |
Sorry, something went wrong.
|
I think a good progression would be:
|
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
@brandtbucher @markshannon what can I do to get this PR moving? @tomasr8 if youd like to review, here's a summary of the PR:
|
Sorry, something went wrong.
|
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 :) |
Sorry, something went wrong.
There was a problem hiding this comment.
Only had time to skim the PR, I'll do a more thorough review this weekend :)
Sorry, something went wrong.
Co-Authored-By: Tomas R. <tomas.roun8@gmail.com>
|
In case it wasn't clear. The helper functions would be: |
Sorry, something went wrong.
Is this the same as this?
|
Sorry, something went wrong.
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.
Sorry, something went wrong.
| } | ||
|
|
||
| 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
Sorry, something went wrong.
| 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)
Sorry, something went wrong.
| write_header, | ||
| Emitter, | ||
| TokenIterator, | ||
| skip_to, |
There was a problem hiding this comment.
See earlier comment.
Sorry, something went wrong.
| ) -> 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()) |
Sorry, something went wrong.
| """ | ||
|
|
||
| import argparse | ||
| import copy |
There was a problem hiding this comment.
Stacks already support copying efficiently and correctly.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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]
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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()) |
Sorry, something went wrong.
| return "JitOptRef " | ||
|
|
||
| def stackref_type_name(var: StackItem) -> str: | ||
| if var.is_array(): |
There was a problem hiding this comment.
| 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.
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again. |
Sorry, something went wrong.
|
Thanks Mark for the review. The code is a lot cleaner now and self-contained. I have made the requested changes; please review again |
Sorry, something went wrong.
|
Thanks for making the requested changes! @markshannon, @tomasr8: please review the changes made to this pull request. |
Sorry, something went wrong.
There was a problem hiding this comment.
That's much better. Thanks.
Sorry, something went wrong.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️Hi! The buildbot iOS ARM64 Simulator 3.x (tier-3) has failed when building commit 695ab61. What do you need to do:
You can take a look at the buildbot page here: https://buildbot.python.org/#/builders/1380/builds/4083 Failed tests:
Failed subtests:
Summary of the results of the build (if available): == Click to see traceback logsTraceback (most recent call last):
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/test/test_socketserver.py", line 222, in test_UnixDatagramServer
self.run_server(socketserver.UnixDatagramServer,
~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
socketserver.DatagramRequestHandler,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
self.dgram_examine)
^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/test/support/threading_helper.py", line 66, in decorator
return func(*args)
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/test/test_socketserver.py", line 133, in run_server
testfunc(svrcls.address_family, addr)
~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/test/test_socketserver.py", line 160, in dgram_examine
buf = data = receive(s, 100)
~~~~~~~^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/test/test_socketserver.py", line 43, in receive
raise RuntimeError("timed out on %r" % (sock,))
RuntimeError: timed out on <socket.socket fd=23, family=1, type=2, proto=0, laddr=./test_python_x18vehwp.sock>
Traceback (most recent call last):
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/threading.py", line 1074, in _bootstrap_inner
self._context.run(self.run)
~~~~~~~~~~~~~~~~~^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/threading.py", line 1016, in run
self._target(*self._args, **self._kwargs)
~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/socketserver.py", line 240, in serve_forever
self._handle_request_noblock()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/socketserver.py", line 320, in _handle_request_noblock
self.handle_error(request, client_address)
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/socketserver.py", line 318, in _handle_request_noblock
self.process_request(request, client_address)
~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/socketserver.py", line 349, in process_request
self.finish_request(request, client_address)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/socketserver.py", line 362, in finish_request
self.RequestHandlerClass(request, client_address, self)
~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/socketserver.py", line 768, in __init__
self.finish()
~~~~~~~~~~~^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/socketserver.py", line 863, in finish
self.socket.sendto(self.wfile.getvalue(), self.client_address)
~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ConnectionResetError: [Errno 54] Connection reset by peer
ERROR
Traceback (most recent call last):
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/test/test_socketserver.py", line 228, in test_ThreadingUnixDatagramServer
self.run_server(socketserver.ThreadingUnixDatagramServer,
~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
socketserver.DatagramRequestHandler,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
self.dgram_examine)
^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/test/support/threading_helper.py", line 66, in decorator
return func(*args)
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/test/test_socketserver.py", line 133, in run_server
testfunc(svrcls.address_family, addr)
~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/test/test_socketserver.py", line 160, in dgram_examine
buf = data = receive(s, 100)
~~~~~~~^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/test/test_socketserver.py", line 43, in receive
raise RuntimeError("timed out on %r" % (sock,))
RuntimeError: timed out on <socket.socket fd=23, family=1, type=2, proto=0, laddr=./test_python_a8m4t39h.sock>
Traceback (most recent call last):
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/threading.py", line 1074, in _bootstrap_inner
self._context.run(self.run)
~~~~~~~~~~~~~~~~~^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/threading.py", line 1016, in run
self._target(*self._args, **self._kwargs)
~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/socketserver.py", line 699, in process_request_thread
self.handle_error(request, client_address)
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/socketserver.py", line 697, in process_request_thread
self.finish_request(request, client_address)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/socketserver.py", line 362, in finish_request
self.RequestHandlerClass(request, client_address, self)
~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/socketserver.py", line 768, in __init__
self.finish()
~~~~~~~~~~~^^
File "/Users/buildbot/Library/Developer/XCTestDevices/3299090E-A190-41F9-9EFB-E7EB12DA3473/data/Containers/Bundle/Application/EDDB3F71-0E9C-40F0-97D8-EB8A643BB963/iOSTestbed.app/python/lib/python3.15/socketserver.py", line 863, in finish
self.socket.sendto(self.wfile.getvalue(), self.client_address)
~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ConnectionResetError: [Errno 54] Connection reset by peer
Warning -- threading_cleanup() failed to clean up threads in 1.0 seconds
Warning -- before: thread count=0, dangling=1
Warning -- after: thread count=1, dangling=4
Warning -- Dangling thread: <_MainThread(MainThread, started 4302897664)>
Warning -- Dangling thread: <Thread(Thread-2933 (process_request_thread), stopped 6218264576)>
Warning -- Dangling thread: <Thread(Thread-2932 (process_request_thread), stopped 6201438208)>
Warning -- Dangling thread: <Thread(<class 'socketserver.ThreadingUnixDatagramServer'> serving, started daemon 6184611840)>
ERROR
|
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
There was a problem hiding this comment.
We should also add None here, as well.
Sorry, something went wrong.
There was a problem hiding this comment.
Ok let's remove it.
Sorry, something went wrong.
…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>
| Back | FazBrowse Home | New Git URL |
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.