KEMBAR78
gh-91276: Make JUMP_IF_TRUE_OR_POP/JUMP_IF_FALSE_OR_POP relative by iritkatriel · Pull Request #32215 · python/cpython · GitHub
Skip to content

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