GeezerVison is showing github.com — `LOAD_FAST` is not always converted to `LOAD_FAST_BORROW` in a `basicblock` · Issue #145629 · python/cpython
LOAD_FAST is not always converted to LOAD_FAST_BORROW in a basicblock #145629
DescriptionBug report
Bug description:
Originally addressed in Issue #144388, the bytecode compiler does not always convert a LOAD_FAST instruction into a LOAD_FAST_BORROW. For example, the following python code, which gets compiled into only one basicblock, has a LOAD_FAST when it could have a LOAD_FAST_BORROW:
from dis import dis
def f(a,b):
return a if a < b else b
dis(f)Byte code
3 RESUME 0
4 LOAD_FAST_BORROW_LOAD_FAST_BORROW 1 (a, b)
COMPARE_OP 18 (bool(<))
POP_JUMP_IF_FALSE 3 (to L1)
NOT_TAKEN
LOAD_FAST_BORROW 0 (a)
RETURN_VALUE
L1: LOAD_FAST 1 (b)
RETURN_VALUE
In contrast, the following expanded version of f does convert all LOAD_FAST instructions into LOAD_FAST_BORROW and has three different basic blocks:
from dis import dis
def g(a,b):
if a < b:
return a
else:
return b
dis(g)Byte code
3 RESUME 0
4 LOAD_FAST_BORROW_LOAD_FAST_BORROW 1 (a, b)
COMPARE_OP 18 (bool(<))
POP_JUMP_IF_FALSE 3 (to L1)
NOT_TAKEN
5 LOAD_FAST_BORROW 0 (a)
RETURN_VALUE
7 L1: LOAD_FAST_BORROW 1 (b)
RETURN_VALUE
The function of interest is optimize_load_fast in Python/flowgraph.c. I have tried to fix this bug but have not been able to yet. All I have found so far is that the LOAD_FAST instruction is misclassified as REF_UNCONSUMED here for the case of function f:
|
for (Py_ssize_t i = 0; i < refs.size; i++) { |
|
ref r = ref_stack_at(&refs, i); |
|
if (r.instr != -1) { |
|
instr_flags[r.instr] |= REF_UNCONSUMED; |
|
} |
|
} |
CPython versions tested on:
CPython main branch
Operating systems tested on:
Linux
Linked PRs
ActivityVanshAgarwal24036 commented on Mar 8, 2026 I investigated in in optimize_load_fast() and found that LOAD_FAST branch is marked REF_UNCONSUMED during reference stack cleanup, which prevents the LOAD_FAST -> LOAD_FAST_BORROW optimization.
From debugging i found that it appears that the refrence remains on the simulated stack when the block ends with RETURN_VALUE, causing it to be incorrectly classified.
Debug line added:
printf("[DEBUG] Marking instr %d as REF_UNCONSUMED (block size %d)\n",
r.instr, block->b_iused);Terminal output:
[DEBUG] Marking instr 0 as REF_UNCONSUMED (block size 1)
I want to confirm that whether the refrence stack should treat RETURN_VALUE as consuming the top stack value during this analysis step. anand-official commented on Mar 8, 2026 brijkapadia commented on Mar 8, 2026 ContributorAuthor More actions @anand-official When I tested your solution, it did not convert the LOAD_FAST into a LOAD_FAST_BORROW from the first example in the original issue.
Would you like me to prepare a PR with this fix?
No.
I want to confirm that whether the refrence stack should treat RETURN_VALUE as consuming the top stack value during this analysis step.
@VanshAgarwal24036 RETURN_VALUE should consume the stack variable. As such, I am not exactly sure about the root cause of this issue. Unless I am missing something, the issue seems very subtle.
I would appreciate any guidance from people who understand the byte code optimizer better than I do. Reacted by (James) Kanin Kearpimy anand-official commented on Mar 8, 2026 Thank you for the detailed bug report. I’ve been looking into this as well and wanted to share my findings and a question.
What I found
The first LOAD_FAST (instruction 0) gets marked REF_UNCONSUMED in the cleanup loop (lines 3006–3011).
This happens because the reference stack still contains an entry when the block ends with RETURN_VALUE. It looks like the simulation never pops for RETURN_VALUE in the main loop.
Why the simple fix didn’t work
I tried popping the reference stack for RETURN_VALUE before cleanup, but that alone doesn’t enable the optimization because the REF_LAST_USE flag for that instruction was already missed in the main loop. We need to handle the pop inside the main loop, just like for BINARY_OP or COMPARE_OP.
The challenge
RETURN_VALUE consumes the top stack value, but that value may or may not be a reference. The current reference stack only tracks references, not all values. To pop correctly, we’d need to know whether the top value is a reference – essentially, we’d need a parallel “value type” stack or a way to deduce it from the instruction that produced it.
Question for core devs
@markshannon, @brandtbucher, @iritkatriel—how is the reference simulation supposed to handle RETURN_VALUE? Is there a known limitation or a simpler way to determine if the top value is a reference without a full value stack? I’d be happy to help implement a fix once we agree on the right approach. VanshAgarwal24036 commented on Mar 9, 2026 I also added debug prints while investigating this and saw that the instruction corresponding to the LOAD_FAST in the else branch gets marked REF_UNCONSUMED during the cleanup loop.
For example:
[DEBUG] Marking instr 0 as REF_UNCONSUMED (block size 1)
This seems to correspond to the LOAD_FAST b before RETURN_VALUE.
So the reference stack still contains the entry when the block ends with RETURN_VALUE, which then gets marked as REF_UNCONSUMED.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|