gh-107557: Remove unnecessary SAVE_IP instructions #108583

gvanrossum · 2023-08-28T20:36:50Z

As a final optimization pass, squeeze out any SAVE_IP instructions that are not needed.

For example, if we have

SAVE_IP
LOAD_FAST
SAVE_IP
LOAD_GLOBAL

we can remove the first SAVE_IP, since LOAD_FAST can neither deoptimize nor error out; but we cannot remove the second SAVE_IP, since LOAD_GLOBAL can error out.

This pass requires new per-opcode flags, HAS_ERROR_FLAG and HAS_DEOPT_FLAG, which must be defined for regular ops as well as for uops. As a bonus we also remove redundant NOP opcodes.

(It may seem suboptimal to call squeeze_stubs() twice -- once after constructing an initial trace if it has stubs, once after removing unneeded uops; but to avoid this we'd need to change a bunch of APIs to return where the gap is. This seems simpler.)

Issue: Tier 2 abstract interpreter for the optimizer #107557

…ove_unneeded_uops()

gvanrossum · 2023-08-28T21:34:47Z

(@Fidget-Spinner I'm not sure if reusing your gh-107557 as the issue in the PR subject is right. I've mostly been skipping news for optimizer/executor based changes because they don't matter to end users and only a few core devs even care. Those interested can look at the commit messages.)

Python/optimizer.c

iritkatriel · 2023-08-29T10:41:04Z

Python/optimizer.c

@@ -372,6 +372,32 @@ static PyTypeObject UOpExecutor_Type = {
    .tp_as_sequence = &uop_as_sequence,
 };

+static int
+squeeze_stubs(


Suggested change

squeeze_stubs(

shift_stubs(

(the name implied to me that something happens to the size of the stubs).

How about move_stubs() (echoing that it uses memmove()).

Python/optimizer.c

iritkatriel · 2023-08-29T10:45:21Z

Python/optimizer.c

+                need_ip = true;
+            }
+        }
+    }


assert here that last_instr >= 0?

No, I can just initialize it to 0 though. If the trace is empty we shouldn't get here but the code here won't do anything.

iritkatriel · 2023-08-29T10:46:39Z

Tools/cases_generator/flags.py

+                or variable_used(instr, "error")
+                or variable_used(instr, "pop_1_error")
+                or variable_used(instr, "exception_unwind")
+                or variable_used(instr, "resume_with_error")


We could make variable_used accept a sequence of strings to check.

We could, but why bother? Plus, I'd kind of like variable_used(instr, X, Y, Z) to look for the sequence of identifiers X Y Z. (And I'd also like to allow keywords, e.g. goto.)

gvanrossum

Thanks for the reviews! I've changed the function name, shortened the comment, and changed the initialization of last_instr. I'll merge once tests pass.

gvanrossum · 2023-08-29T15:59:20Z

Python/optimizer.c

@@ -372,6 +372,32 @@ static PyTypeObject UOpExecutor_Type = {
    .tp_as_sequence = &uop_as_sequence,
 };

+static int
+squeeze_stubs(


How about move_stubs() (echoing that it uses memmove()).

gvanrossum · 2023-08-29T16:06:26Z

Python/optimizer.c

+                need_ip = true;
+            }
+        }
+    }


No, I can just initialize it to 0 though. If the trace is empty we shouldn't get here but the code here won't do anything.

gvanrossum · 2023-08-29T16:09:35Z

Tools/cases_generator/flags.py

+                or variable_used(instr, "error")
+                or variable_used(instr, "pop_1_error")
+                or variable_used(instr, "exception_unwind")
+                or variable_used(instr, "resume_with_error")


We could, but why bother? Plus, I'd kind of like variable_used(instr, X, Y, Z) to look for the sequence of identifiers X Y Z. (And I'd also like to allow keywords, e.g. goto.)

bedevere-bot · 2023-08-29T17:18:28Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Windows10 3.x has failed when building commit 4f22152.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/146/builds/6164) and take a look at the build logs.
Check if the failure is related to this commit (4f22152) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/146/builds/6164

Failed tests:

test.test_concurrent_futures.test_shutdown

Summary of the results of the build (if available):

== Tests result: FAILURE then SUCCESS ==

426 tests OK.

10 slowest tests:

test_math: 6 min 21 sec
test_peg_generator: 2 min 37 sec
test_wmi: 2 min 20 sec
test_tokenize: 2 min 5 sec
test.test_multiprocessing_spawn.test_processes: 2 min 2 sec
test_unparse: 2 min 2 sec
test_compileall: 1 min 31 sec
test_mmap: 1 min 29 sec
test_pickle: 1 min 26 sec
test_unicodedata: 1 min 25 sec

37 tests skipped:
test.test_asyncio.test_unix_events
test.test_multiprocessing_fork.test_manager
test.test_multiprocessing_fork.test_misc
test.test_multiprocessing_fork.test_processes
test.test_multiprocessing_fork.test_threads
test.test_multiprocessing_forkserver.test_manager
test.test_multiprocessing_forkserver.test_misc
test.test_multiprocessing_forkserver.test_processes
test.test_multiprocessing_forkserver.test_threads test_curses
test_dbm_gnu test_dbm_ndbm test_devpoll test_epoll test_fcntl
test_fork1 test_gdb test_grp test_ioctl test_kqueue test_openpty
test_perf_profiler test_perfmaps test_poll test_posix test_pty
test_pwd test_readline test_resource test_syslog
test_threadsignals test_wait3 test_wait4 test_xxlimited
test_xxtestfuzz test_zipfile64 test_zoneinfo

1 re-run test:
test.test_concurrent_futures.test_shutdown

Total duration: 22 min 32 sec

Click to see traceback logs

Traceback (most recent call last):
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\__init__.py", line 203, in _force_run
    return func(*args)
           ^^^^^^^^^^^
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\\buildarea\\3.x.bolen-windows10\\build\\build\\test_python_7492�\\test_python_worker_10044�'


Traceback (most recent call last):
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\runpy.py", line 198, in _run_module_as_main
    return _run_code(code, main_globals, None,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\runpy.py", line 88, in _run_code
    exec(code, run_globals)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\__main__.py", line 2, in <module>
    main()
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\libregrtest\main.py", line 826, in main
    Regrtest().main(tests=tests, **kwargs)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\libregrtest\main.py", line 756, in main
    with os_helper.temp_cwd(test_cwd, quiet=True):
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\contextlib.py", line 159, in __exit__
    self.gen.throw(value)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 531, in temp_cwd
    with temp_dir(path=name, quiet=quiet) as temp_path:
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\contextlib.py", line 159, in __exit__
    self.gen.throw(value)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 485, in temp_dir
    rmtree(path)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 442, in rmtree
    _rmtree(path)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 385, in _rmtree
    _waitfor(_rmtree_inner, path, waitall=True)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 330, in _waitfor
    func(pathname)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 382, in _rmtree_inner
    _force_run(fullname, os.rmdir, fullname)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\__init__.py", line 214, in _force_run
    return func(*args)
           ^^^^^^^^^^^
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\\buildarea\\3.x.bolen-windows10\\build\\build\\test_python_7492�\\test_python_worker_10044�'


Traceback (most recent call last):
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 480, in temp_dir
    yield path
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 533, in temp_cwd
    yield cwd_dir
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\libregrtest\main.py", line 762, in main
    self._main(tests, kwargs)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\libregrtest\main.py", line 821, in _main
    sys.exit(0)
SystemExit: 0

gvanrossum · 2023-08-29T17:31:44Z

Benchmarking results show this would be 4% slower than main if the optimizer was always on (it isn't).

gvanrossum added 3 commits August 28, 2023 10:59

Add HAS_ERROR_FLAG and HAS_DEOPT_FLAG

f289749

Write metadata for all ops and all instrs

290f955

Squeeze out unneeded SAVE_IP from traces

d9258d8

bedevere-bot mentioned this pull request Aug 28, 2023

Tier 2 abstract interpreter for the optimizer #107557

Closed

3 tasks

gvanrossum requested review from Fidget-Spinner and iritkatriel August 28, 2023 20:37

gvanrossum added 3 commits August 28, 2023 14:04

Factor squeeze_stubs() out of translate_bytecode_to_trace()

51eaa2f

Use squeeze_stubs() instead of inserting NOPs into gap created by rem…

4c5c38d

…ove_unneeded_uops()

When there's no gap we can set trace_length = buffer_size

869ae8d

gvanrossum added the skip news label Aug 28, 2023

gvanrossum added 2 commits August 28, 2023 14:30

Always remove NOPs, even if no new ones were added

bf9fefd

Change meaning of last_instr to be base-0

8d31dc0

Tweaks to debug output

afb7cc1

gvanrossum marked this pull request as ready for review August 28, 2023 22:14

bedevere-bot added the awaiting core review label Aug 28, 2023

Fidget-Spinner approved these changes Aug 29, 2023

View reviewed changes

Python/optimizer.c Show resolved Hide resolved

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 29, 2023

iritkatriel reviewed Aug 29, 2023

View reviewed changes

gvanrossum added 3 commits August 29, 2023 08:59

Rename squeeze_stubs() to move_stubs()

67b9e6b

Shorten comment

ad5c50f

Initialize last_instr to 0, not -1

ebce668

gvanrossum commented Aug 29, 2023

View reviewed changes

gvanrossum enabled auto-merge (squash) August 29, 2023 16:15

gvanrossum merged commit 4f22152 into python:main Aug 29, 2023

bedevere-bot removed the awaiting merge label Aug 29, 2023

gvanrossum deleted the omit-save-ip branch August 29, 2023 17:30

gvanrossum mentioned this pull request Sep 4, 2023

We need to change the contract and interface of _PyExecutorObject and _PyOptimizerObject #108866

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-107557: Remove unnecessary SAVE_IP instructions #108583

gh-107557: Remove unnecessary SAVE_IP instructions #108583

gvanrossum commented Aug 28, 2023 •

edited

Loading

gvanrossum commented Aug 28, 2023

iritkatriel Aug 29, 2023

gvanrossum Aug 29, 2023

iritkatriel Aug 29, 2023

gvanrossum Aug 29, 2023

iritkatriel Aug 29, 2023

gvanrossum Aug 29, 2023

gvanrossum left a comment

gvanrossum Aug 29, 2023

gvanrossum Aug 29, 2023

gvanrossum Aug 29, 2023

bedevere-bot commented Aug 29, 2023

gvanrossum commented Aug 29, 2023

gh-107557: Remove unnecessary SAVE_IP instructions #108583

gh-107557: Remove unnecessary SAVE_IP instructions #108583

Conversation

gvanrossum commented Aug 28, 2023 • edited Loading

gvanrossum commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 29, 2023

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

gvanrossum commented Aug 29, 2023

gvanrossum commented Aug 28, 2023 •

edited

Loading