Skip to content

gh-91276: Make JUMP_IF_TRUE_OR_POP/JUMP_IF_FALSE_OR_POP relative #32215

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 7 commits into from
Apr 15, 2022

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Mar 31, 2022

@markshannon
Copy link
Member

Are you completely sure it is impossible for the compiler to generate a backwards branch for these instructions?

I can't produce a case now, but I can create a case that could jump backwards with the right compiler change.

def f(a,b):
    while x:= a or b:
        pass

compiles to:

  1           0 RESUME                   0

  2           2 LOAD_FAST                0 (a)
              4 JUMP_IF_TRUE_OR_POP      4 (to 8)
              6 LOAD_FAST                1 (b)
        >>    8 COPY                     1
             10 STORE_FAST               2 (x)
             12 POP_JUMP_IF_FALSE       16 (to 32)

  3     >>   14 NOP

  2          16 LOAD_FAST                0 (a)
             18 JUMP_IF_TRUE_OR_POP     11 (to 22)
             20 LOAD_FAST                1 (b)
        >>   22 COPY                     1
             24 STORE_FAST               2 (x)
             26 POP_JUMP_IF_TRUE         7 (to 14)
             28 LOAD_CONST               0 (None)
             30 RETURN_VALUE
        >>   32 LOAD_CONST               0 (None)
             34 RETURN_VALUE

The compiler could, in theory, retarget the branch at instruction 18 so that it jumps to 8.
If it did, then there would be a JUMP_IF_TRUE_OR_POP jumping backwards.

@markshannon
Copy link
Member

We could handle this in the assembler, I guess.

JUMP_IF_TRUE_OR_POP back

Would become

COPY 1
POP_JUMP_IF_FALSE back
POP_TOP

As long as backward jumps are rare (and they will be), then this wouldn't add any real overhead.

@iritkatriel iritkatriel marked this pull request as draft March 31, 2022 18:18
@iritkatriel
Copy link
Member Author

Are you completely sure it is impossible for the compiler to generate a backwards branch for these instructions?

I put an assertion and all tests passed. But yeah, we can make it more rubust as you suggest.

@iritkatriel
Copy link
Member Author

We could handle this in the assembler, I guess.

JUMP_IF_TRUE_OR_POP back

Would become

COPY 1
POP_JUMP_IF_FALSE back
POP_TOP

As long as backward jumps are rare (and they will be), then this wouldn't add any real overhead.

There is the problem of how to test this, since we don't know how to currently make the compiler create a backwards jump with this opcode.

@ghost
Copy link

ghost commented Apr 11, 2022

Commit authors are required to sign the Contributor License Agreement.
CLA not signed

@iritkatriel iritkatriel marked this pull request as ready for review April 11, 2022 18:49
@iritkatriel iritkatriel changed the title bpo-47120: Make JUMP_IF_TRUE_OR_POP/JUMP_IF_FALSE_OR_POP relative gh-91276: Make JUMP_IF_TRUE_OR_POP/JUMP_IF_FALSE_OR_POP relative Apr 12, 2022
@iritkatriel
Copy link
Member Author

Closing and reopening to kick the bots.

@iritkatriel iritkatriel reopened this Apr 12, 2022
@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 12, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit ec65324 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 12, 2022
@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 13, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 2dfe713 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 13, 2022
@iritkatriel
Copy link
Member Author

Buildbots are happy.

@iritkatriel iritkatriel merged commit ea2ae02 into python:main Apr 15, 2022
@iritkatriel iritkatriel deleted the jump_or_pop branch May 20, 2022 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants