GH-109214: _SET_IP before _PUSH_FRAME (but not _POP_FRAME)#111001
GH-109214: _SET_IP before _PUSH_FRAME (but not _POP_FRAME)#111001brandtbucher merged 3 commits intopython:mainfrom
_SET_IP before _PUSH_FRAME (but not _POP_FRAME)#111001Conversation
brandtbucher
added
interpreter-core
gvanrossum
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
I can't say I am less happy with this than with the situation before the bug was introduced, but I am also not sure I am happier. It feels as if the various special uops SET_IP, SAVE_CURRENT_IP, POP/PUSH_FRAME can still use more work (not in this PR though).
| case OPARG_SET_IP: // op==_SET_IP; oparg=next instr | ||
| case OPARG_SET_IP: // uop=_SET_IP; oparg=next_instr-1 | ||
| // The number of caches is smuggled in via offset: | ||
| assert(offset == _PyOpcode_Caches[_PyOpcode_Deopt[opcode]]); |
There was a problem hiding this comment.
If this is always it, why do we need to smuggle it in via offset? Also it's beginning to look like the offset should just be made one larger in the code generator, so we can drop the ugly + 1 on line 654 above.
|
I agree with your comments. I'll land this now to unblock tier two, and work on a follow-up PR to clean up this IP stuff further. |
Maybe wait until Irit's PR lands. It touches on all these things. |
|
Okay, I will. |
2 participants
#110755 inadvertently allowed the tier two optimizer to remove instruction pointer updates before frame pushes, which breaks the following return. This updates
remove_unneeded_uopsto recognize_PUSH_FRAMEas a uop that requires an up-to-date instruction pointer on the frame.While investigating this, I realized that we actually don't need to set the instruction pointer before returning from a frame (since the frame is unlinked and torn down immediately after). So I've also removed
_SAVE_CURRENT_IPfrom both of the return opcodes, which is a nice little win for tier one.Also, remove some dead code and comment/assert a non-obvious bit of code during trace projection.