gh-106581: Fix two bugs in the code generator's copy optimization - #108380
gh-106581: Fix two bugs in the code generator's copy optimization#108380gvanrossum merged 5 commits into
Conversation
Sorry, something went wrong.
I was comparing the last preceding poke with the *last* peek, rather than the *first* peek. Unfortunately this bug obscured another bug, which I have yet to fix: When the last preceding poke is UNUSED, the first peek disappears, leaving the variable unassigned.
Rename CopyEffect to CopyItem. Change CopyItem to contain StackItems instead of StackEffects. Update those StackItems when adjusting the manager higher or lower. Assert that those StackItems' offsets are equivalent.
|
I apologize, when using this to split CALL_BOUND_METHOD_EXACT_ARGS I found there are more things wrong with it. So I added another commit. This mainly looks for cases of the form copy(a, unused) followed by copy(unused, b) and changes the latter into copy(a, b). (The former is effectively a no-op already.) Right now this has only a minor effect (it removes one more redundant variable load from the stack) but when splitting CALL_BOUND_METHOD_EXACT_ARGS it is of the utmost importance. However, I don't want to put that split in this same PR. |
Sorry, something went wrong.
iritkatriel
left a comment
There was a problem hiding this comment.
Minor tweaks.
Sorry, something went wrong.
(I had different code in one of the branches during debugging. :-) Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
When replacing matched pairs of push/pop (poke/peek), I was matching up the last poke in the previous "manager" with the wrong peek in the new one, causing some copying opportunities to be missed.
Fixing this revealed a more serious bug where we could end up skipping the copy of an 'unused' poke to a used peek, leaving the variable uninitialized. The fix for this required me to keep track of the original StackItems in the CopyEffect object, and use that to reconstruct the source when copying from 'unused'.
The net result so far is that this found two redundant initializations.