Skip to content

LOAD_FAST_BORROW not being used even when safe to do so, if value is live at BB end. #133672

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

Open
markshannon opened this issue May 8, 2025 · 2 comments
Labels
3.15 new features, bugs and security fixes performance Performance or resource usage

Comments

@markshannon
Copy link
Member

markshannon commented May 8, 2025

This function

def f(x, y, c):
    return 1 + (x if c else y)

compiles to

  1           RESUME                   0

  2           LOAD_SMALL_INT           1
              LOAD_FAST_BORROW         2 (c)
              TO_BOOL
              POP_JUMP_IF_FALSE        9 (to L1)
              NOT_TAKEN
              LOAD_FAST_BORROW         0 (x)
              BINARY_OP                0 (+)
              RETURN_VALUE
      L1:     LOAD_FAST                1 (y)
              BINARY_OP                0 (+)
              RETURN_VALUE

Note that the load of y uses LOAD_FAST even though LOAD_FAST_BORROW is safe.

This becomes important with virtual iterators as the iterable for the loop is live at BB end.

Linked PRs

@markshannon
Copy link
Member Author

@iritkatriel
@mpage

@markshannon markshannon added performance Performance or resource usage 3.15 new features, bugs and security fixes labels May 8, 2025
@ljfp
Copy link
Contributor

ljfp commented May 9, 2025

@markshannon I've created this PR to modify the optimization condition in Python/flowgraph.c. The goal is to allow LOAD_FAST to be optimized to LOAD_FAST_BORROW even when the reference is still on the stack at the end of a basic block (i.e., the REF_UNCONSUMED flag is set).

If I got this right, this should be safe as long as the reference is not killed (SUPPORT_KILLED flag isn't set) and not stored as a local (STORED_AS_LOCAL flag isn't set).

With these changes, y in your original example now correctly uses LOAD_FAST_BORROW:

Python 3.15.0a0 (heads/fix-issue-133672:f038e7f472e, May  9 2025, 01:25:23) [GCC 15.1.1 20250425] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> def f(x, y, c):
...     return 1 + (x if c else y)
... 
>>> import dis
>>> dis.dis(f)
  1           RESUME                   0

  2           LOAD_SMALL_INT           1
              LOAD_FAST_BORROW         2 (c)
              TO_BOOL
              POP_JUMP_IF_FALSE        9 (to L1)
              NOT_TAKEN
              LOAD_FAST_BORROW         0 (x)
              BINARY_OP                0 (+)
              RETURN_VALUE
      L1:     LOAD_FAST_BORROW         1 (y)
              BINARY_OP                0 (+)
              RETURN_VALUE

Could you take a look when you have a moment and confirm if this aligns with what you had in mind?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.15 new features, bugs and security fixes performance Performance or resource usage
Projects
None yet
Development

No branches or pull requests

2 participants