Skip to content

gh-131798: Small improvements to remove_unneeded_uops #134554

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

Merged
merged 1 commit into from
May 23, 2025

Conversation

tomasr8
Copy link
Member

@tomasr8 tomasr8 commented May 22, 2025

  • Add _CHECK_PERIODIC to the list of skipped instructions. I believe this is safe to do since this instruction does not modify the stack. It is also relatively common so I think it's worth it to skip it.
  • Add an extra guard to the optimizer.

@@ -617,7 +618,7 @@ remove_unneeded_uops(_PyUOpInstruction *buffer, int buffer_size)
while (op_skip[last->opcode]) {
last--;
}
if (op_without_push[last->opcode]) {
if (op_without_push[last->opcode] && op_without_pop[opcode]) {
last->opcode = op_without_push[last->opcode];
opcode = buffer[pc].opcode = op_without_pop[opcode];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we are here, we know that op_without_pop[opcode] || op_without_pop_null[opcode] is true. But we should also make sure that op_without_pop[opcode] itself is true, otherwise it is possible to accidentally write 0 as the opcode.

@tomasr8 tomasr8 requested a review from brandtbucher May 22, 2025 23:26
@Fidget-Spinner
Copy link
Member

  • Add _CHECK_PERIODIC to the list of skipped instructions. I believe this is safe to do since this instruction does not modify the stack. It is also relatively common so I think it's worth it to skip it.

I read the code but just checking that I understand it right: op_skip means the instruction is still emitted, just that we "skip" it when trying to optimize away redundant push/pops right? If so that's ok the PR LGTM.

@tomasr8
Copy link
Member Author

tomasr8 commented May 23, 2025

  • Add _CHECK_PERIODIC to the list of skipped instructions. I believe this is safe to do since this instruction does not modify the stack. It is also relatively common so I think it's worth it to skip it.

I read the code but just checking that I understand it right: op_skip means the instruction is still emitted, just that we "skip" it when trying to optimize away redundant push/pops right? If so that's ok the PR LGTM.

Yes, that's exactly the case! This allows us to optimize for instance:

_LOAD_CONST
_CHECK_PERIODIC
_POP_TOP

into just

_NOP
_CHECK_PERIODIC
_NOP

@Fidget-Spinner Fidget-Spinner merged commit 71dea74 into python:main May 23, 2025
61 checks passed
@tomasr8 tomasr8 deleted the jit-remove-uops branch May 23, 2025 13:04
@bedevere-bot
Copy link

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

Hi! The buildbot AMD64 Fedora Stable Refleaks 3.x (tier-1) has failed when building commit 71dea74.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/320/builds/2497) and take a look at the build logs.
  4. Check if the failure is related to this commit (71dea74) or if it is a false positive.
  5. 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/#/builders/320/builds/2497

Failed tests:

  • test.test_multiprocessing_fork.test_processes

Failed subtests:

  • test_interrupt - test.test_multiprocessing_fork.test_processes.WithProcessesTestProcess.test_interrupt

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

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/process.py", line 320, in _bootstrap
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 518, in _sleep_some_event
    time.sleep(100)
    ~~~~~~~~~~^^^^^
KeyboardInterrupt
k


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 588, in test_interrupt
    exitcode = self._kill_process(multiprocessing.Process.interrupt)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 570, in _kill_process
    self.assertEqual(join(), None)
                     ~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 250, in __call__
    return self.func(*args, **kwds)
           ~~~~~~~~~^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/process.py", line 156, in join
    res = self._popen.wait(timeout)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/popen_fork.py", line 44, in wait
    return self.poll(os.WNOHANG if timeout == 0.0 else 0)
           ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/popen_fork.py", line 28, in poll
    pid, sts = os.waitpid(self.pid, flag)
               ~~~~~~~~~~^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 566, in handler
    raise RuntimeError('join took too long: %s' % p)
RuntimeError: join took too long: <Process name='Process-3' pid=1325402 parent=1325398 started daemon>


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/process.py", line 320, in _bootstrap
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 517, in _sleep_some_event
    event.set()
    ~~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/synchronize.py", line 344, in set
    with self._cond:
         ^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/synchronize.py", line 242, in __exit__
    return self._lock.__exit__(*args)
           ~~~~~~~~~~~~~~~~~~~^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/synchronize.py", line 100, in __exit__
    return self._semlock.__exit__(*args)
           ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^
KeyboardInterrupt
k


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 588, in test_interrupt
    exitcode = self._kill_process(multiprocessing.Process.interrupt)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 570, in _kill_process
    self.assertEqual(join(), None)
                     ~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 250, in __call__
    return self.func(*args, **kwds)
           ~~~~~~~~~^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/process.py", line 156, in join
    res = self._popen.wait(timeout)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/popen_fork.py", line 44, in wait
    return self.poll(os.WNOHANG if timeout == 0.0 else 0)
           ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/popen_fork.py", line 28, in poll
    pid, sts = os.waitpid(self.pid, flag)
               ~~~~~~~~~~^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 566, in handler
    raise RuntimeError('join took too long: %s' % p)
RuntimeError: join took too long: <Process name='Process-1706' pid=1247987 parent=1230274 started daemon>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants