◐ Shell
reader mode source ↗
Skip to content

GH-128563: Generate opcode = ... in instructions that need opcode#129608

Merged
markshannon merged 3 commits into
python:mainfrom
faster-cpython:make-opcode-static
Feb 3, 2025
Merged

GH-128563: Generate opcode = ... in instructions that need opcode#129608
markshannon merged 3 commits into
python:mainfrom
faster-cpython:make-opcode-static

Conversation

@markshannon

@markshannon markshannon commented Feb 3, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

This PR:

  • In instruction bodies that use opcode, generate opcode = ... at the start of the instruction
  • Fix instructions that relied to the dynamic value of opcode by using micro-ops
  • Remove GO_TO_INSTRUCTION macro as it is no longer needed.

This means that we no longer need to pass the opcode parameter in tailcalls.

Performance is neutral.

@bedevere-app bedevere-app Bot mentioned this pull request Feb 3, 2025
Closed
@Fidget-Spinner Fidget-Spinner changed the title GH-128563: Generate opocde = ... in instructions that need opcode Feb 3, 2025
Fidget-Spinner
Fidget-Spinner approved these changes Feb 3, 2025
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

I have a feeling if you remove the uint8_t opcode in the outer loop, and just define it as int opcode = XXX in each bytecode case, there will be a speedup in the normal interpreter as well (or at least, there will be less pressure on the register allocator), as it will free up a single outer variable that is live across all basic blocks.

@markshannon

markshannon commented Feb 3, 2025

Copy link
Copy Markdown
Member Author

I have a feeling if you remove the uint8_t opcode in the outer loop, and just define it as int opcode = XXX in each bytecode case, there will be a speedup in the normal interpreter as well (or at least, there will be less pressure on the register allocator), as it will free up a single outer variable that is live across all basic blocks.

We can't do that for the switch case, but we could for computed gotos.

@Fidget-Spinner

Fidget-Spinner commented Feb 3, 2025

Copy link
Copy Markdown
Member

Don't think it's worth diverging the two over that. Let's just keep it simple then and do this.

@markshannon markshannon merged commit 75b628a into python:main Feb 3, 2025
@bedevere-bot

bedevere-bot commented Feb 3, 2025

Copy link
Copy Markdown

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

Hi! The buildbot aarch64 Fedora Stable Clang 3.x has failed when building commit 75b628a.

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/234/builds/6984) and take a look at the build logs.
  4. Check if the failure is related to this commit (75b628a) 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/234/builds/6984

Failed tests:

  • test_interpreters

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-aarch64.clang/build/Lib/threading.py", line 1054, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.clang/build/Lib/threading.py", line 996, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.clang/build/Lib/test/test_interpreters/test_stress.py", line 47, in run
    interp = interpreters.create()
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.clang/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(reqrefs=True)
interpreters.InterpreterError: interpreter creation failed
k


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.clang/build/Lib/threading.py", line 1054, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.clang/build/Lib/threading.py", line 996, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.clang/build/Lib/test/test_interpreters/test_stress.py", line 30, in task
    interp = interpreters.create()
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.clang/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(reqrefs=True)
interpreters.InterpreterError: interpreter creation failed
k

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Feb 7, 2025
@markshannon @srinivasreddy
pythonGH-128563: Generate opcode = ... in instructions that need `o… …
5deacc1 
…pcode` (pythonGH-129608)

* Remove support for GO_TO_INSTRUCTION
@markshannon markshannon deleted the make-opcode-static branch February 27, 2025 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@markshannon @Fidget-Spinner @bedevere-bot