gh-98831: Modernize the FOR_ITER family of instructions - #101626
Conversation
Sorry, something went wrong.
Fidget-Spinner
left a comment
There was a problem hiding this comment.
Code looks good. Just one question
Sorry, something went wrong.
| if (it->it_index < PyList_GET_SIZE(seq)) { | ||
| PyObject *next = PyList_GET_ITEM(seq, it->it_index++); | ||
| PUSH(Py_NewRef(next)); | ||
| JUMPBY(INLINE_CACHE_ENTRIES_FOR_ITER); |
There was a problem hiding this comment.
I'm a little confused, why do we not need to JUMPBY INLINE_CACHE_ENTRIES_FOR_ITER anymore?
Sorry, something went wrong.
There was a problem hiding this comment.
I think this is the JUMPBY(1); in line 2688.
Sorry, something went wrong.
There was a problem hiding this comment.
Ahh right. I forgot that the inline cache entries for FOR_ITER is just the counter. Thanks Irit!
Sorry, something went wrong.
| FOR_ITER_GEN, | ||
| }; | ||
|
|
||
| inst(FOR_ITER, (unused/1, iter -- iter, next)) { |
There was a problem hiding this comment.
Should this be counter/1 instead of unused/1, and then the counter used in the body?
Sorry, something went wrong.
There was a problem hiding this comment.
In all cases where the counter is manipulated using ADAPTIVE_COUNTER_IS_ZERO / DECREMENT_ADAPTIVE_COUNTER I'm leaving this idiom unchanged -- the generator won't let you write back to cache entries, so it would become asymmetrical, and the common case is DECREMENT_ADAPTIVE_COUNTER anyway.
Sorry, something went wrong.
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
This is somewhat tricky because most of the family members (with the exception of FOR_ITER_GEN) are actually super-instructions that jump over the END_FOR instruction that is the target of the jump (which pops two stack items). In addition, FOR_ITER_RANGE also jumps over the STORE_FAST that follows it when the range is not exhausted.
The cache and stack effects documented in the DSL don't acknowledge the existence of these optimizations. (This will bite us when we try to incorporate these instructions in a level 2 interpreter, we'll have to deal with it then.)