Skip to content
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

GH-111485: Remove some special cases from the code generator and bytecodes.c #111540

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Oct 31, 2023

  • Removes a bunch of special cases for tier-1-only instruction detection, making them explicit in bytecodes.c with TIER_ONE_ONLY.
  • Don't emit or check formats for psuedo-instructions. They don't exist in the bytecode, so don't have a format.
  • Make the counter in JUMP_BACKWARD explicit
  • Fix error handling in MAKE_CELL so it can be executed in tier 2.

@markshannon markshannon changed the title Remove some special cases from the code generator and bytecodes.c GH-111485: Remove some special cases from the code generator and bytecodes.c Oct 31, 2023
@markshannon markshannon merged commit 2904d99 into python:main Oct 31, 2023
33 checks passed
"exception_unwind",
"import_from",
"import_name",
"_PyObject_CallNoArgs", # Proxy for BEFORE_WITH
Copy link
Member

@brandtbucher brandtbucher Nov 2, 2023

Choose a reason for hiding this comment

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

@gvanrossum, do we know why we were special-casing BEFORE_WITH like this?

I ask because it looks like this PR made BEFORE_ASYNC_WITH (which also uses _PyObject_CallNoArgs) a viable uop. It seems fine, but maybe I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

@gvanrossum, do we know why we were special-casing BEFORE_WITH like this?

I ask because it looks like this PR made BEFORE_ASYNC_WITH (which also uses _PyObject_CallNoArgs) a viable uop. It seems fine, but maybe I'm missing something.

Urgh, I have no idea. :-( Agreed that both seem fine. Either both should have the TIER_ONE_ONLY marker, or neither should. Let me dig through some history:

It's been like this since the cases_generator code first landed (#105924). And even then, commenting out that line makes BEFORE_*WITH appear in the output and it compiles just fine. In the PR I found this commit though:
e3471da

The commit message gives a clue:

Fix test_memoryview by excluding BEFORE_WITH

Unclear what's wrong with that opcode."

Commenting out the line, regenerating, and then running ./python.exe -Xuops -m test test_memoryview gives this error:

test test_memoryview failed -- Traceback (most recent call last):
  File "/Users/guido/cpython/Lib/test/test_memoryview.py", line 288, in test_contextmanager
    self._check_released(m, tp)
  File "/Users/guido/cpython/Lib/test/test_memoryview.py", line 273, in _check_released
    with m:
         ^
ValueError: operation forbidden on released memoryview object

Doing the same on main, however, causes no crashes. Quite possibly whatever was defective in the original code generator and Tier 2 interpreter has since been fixed, and the TIER_ONE_ONLY marker can safely be removed from BEFORE_WITH. I don't feel like doing a bisection to figure out the exact commit that made the test_memoryview failure go away.

FullteaR pushed a commit to FullteaR/cpython that referenced this pull request Nov 3, 2023
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@markshannon markshannon deleted the make-tiers-explicit branch August 6, 2024 10:17
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
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.

4 participants