Repository navigation
Tier2 peephole optimization: remove extraneous _CHECK_STACK_SPACE ops #116168
Description
Activity
This might need some labels. I also looked around for a parent ticket, but didn't find anything particularly obvious
I will link to #115419
I don't know anything about the plan here, so this may not be news. :-) Couldn't you check for the needed stack space once at the beginning of the executor? And if there's a loop, lift it out of there. (And if it fits within the stack space of the code object already, drop it altogether.)
Makes sense. Let's go with consolidating them first then another follow-up PR for lifting. Lifting stuff out of loops requires
_JUMP_ABSOLUTEand I can just point to the code from my old PR to copy. (Though the heuristics need someone else to work on it).Is anyone working on this? Otherwise I'd like to give it a go.
@gvanrossum I think @lazorchakp might be working on this, @lazorchakp do you mind if Guido takes over? If you have something with an implementation up but no tests that's okay, you can just submit a draft PR and Guido can probably take over if you don't mind.
I'd also be happy to mentor @lazorchakp !
Sorry everyone - I've been busier than anticipated and haven't made concrete progress on this yet. If it's alright though, I'd still like to try it, and I should be able to get a draft up tonight if that works. @Fidget-Spinner has given me some guidance over email, but I'd gladly accept as much mentoring as you both are willing to provide 🙂
That being said, @gvanrossum if you'd rather go for it yourself, that's quite alright! Just let me know.
No, I'd love to have you do it!
My guess is that the critical trick should be to change the uop that checks for stack space to take a number indicating the amount of stack space needed, rather than digging that value out of the code object (which must be retrieved from the function object, which must be dug out of the stack at position
-2-oparg, IIRC).Maybe this number can be put in its operand field by the initial trace projection pass (which needs the code object anyway). Then a later dedicated pass can combine stack checks as long as they are within the same "scope" (i.e.,
_PUSH_FRAME/_POP_FRAMEpair).That sounds great. It makes sense to me that we'd now need
_CHECK_STACK_SPACEto know up-front how much space it's looking for. I'll see if I can take your suggestion and modify the uop so that this information is available for the optimization pass.Reacted by Guido van Rossum- added a commit that references this issue
on Mar 21, 2024 I'm at the point where I have the operand for each
_CHECK_STACK_SPACEbeing correctly assigned to its corresponding function's framesize (pushed a rough branch). Before I implement the actual optimization though, I want to make sure I'm reasoning about this correctly - I'm making some heavy assumptions about the underlying machinery. Are the following cases correct? (other ops removed for simplicity):
CASE 1 - sequential function calls:
_CHECK_STACK_SPACE A _PUSH_FRAME _POP_FRAME _CHECK_STACK_SPACE B _PUSH_FRAME _POP_FRAMEproposed optimization:
_CHECK_STACK_SPACE max(A, B) _PUSH_FRAME _POP_FRAME _PUSH_FRAME _POP_FRAME
CASE 2 - nested function calls:
_CHECK_STACK_SPACE A _PUSH_FRAME _CHECK_STACK_SPACE B _PUSH_FRAME _POP_FRAME _POP_FRAMEproposed optimization:
_CHECK_STACK_SPACE A + B _PUSH_FRAME _PUSH_FRAME _POP_FRAME _POP_FRAME
If these cases are correct, I should be able to combine everything into a single _CHECK_STACK_SPACE uop per trace, as @gvanrossum mentioned above.
13 remaining items
Still have to iterate with the pipeline a bit in my draft PR. I'll move it to Open and send an update here when it's officially ready for review.
Just opened #117242 for review
- added 4 commits that reference this issue
on Mar 29, 2024 This is done -- thanks again!
Implement a new peephole optimization for the tier2 optimizer that removes _CHECK_STACK_SPACE if we see that this check is already present
Linked PRs
_CHECK_STACK_SPACEuops #117242