Add uops support to instr_ptr branch #55
Conversation
I'm unhappy with how this turned out; it's too complicated and unprincipled. I feel I don't know which invariants apply when to frame->instr_ptr and frame->next_instr_offset, and the clever exceptions in the code generator and the Tier 1-2 translator. It feels too much like programming by random modifications.
Python/bytecodes.c
Outdated
| #if TIER_TWO | ||
| frame->next_instr_offset = oparg; | ||
| #endif | ||
| assert(frame->next_instr_offset != 0); |
There was a problem hiding this comment.
Is this true for a JUMP_BACKWARD with offset 0?
>>> def f():
... while True: pass
...
>>> dis.dis(f)
1 0 RESUME 0
2 >> 2 JUMP_BACKWARD 2 (to 2)
There was a problem hiding this comment.
Oh, interesting counterexample. In fact, it seems this code gets the translator in a loop, repeating two uops forever:
ADD_TO_TRACE(_JUMP_TO_TOP, 0, 0)
ADD_TO_TRACE(_SET_IP, 1, 0)
I'll have to see if that's new or existing.
There was a problem hiding this comment.
That's not a new bug -- I can trigger it in main as well. So it's a good find! Until I fix it, it's immaterial whether that assert would fail, since such code can't be translated. But I'll remove it, since it serves no real purpose except to reveal my misunderstanding. :-)
There was a problem hiding this comment.
Oh wait. That's not really a bug, I didn't set the debug output level high enough (the ADD_TO_TRACE(...) debug output appears at debug level 2, but the message "No trace for ..." only shows at debug level 4 or higher). The translator is fine; it stops after adding 2 uops and then the trace is rejected because it's too short. The repetitive output is because we don't (yet) have exponential back-off in the translator -- it tries optimizing every 20 executions of the JUMP_BACKWARD, getting the same outcome each time:
Optimizing f (/Users/guido/cpython/t.py:1) at byte offset 2
ADD_TO_TRACE(_SET_IP, 1, 0)
ADD_TO_TRACE(_JUMP_TO_TOP, 0, 0)
No trace for f (/Users/guido/cpython/t.py:1) at byte offset 2
I'll just push a commit that removes the assert, even though no trace should ever exhibit that problem.
@iritkatriel This should be squash-merged into pythongh-109095; it makes all the uops tests pass (and more). Despite my misgivings, I think this is the way to go.
Ref: python#109095 (comment)