Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
GH-118093: Add tier two support forBINARY_OP_INPLACE_ADD_UNICODE#122253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
GH-118093: Add tier two support forBINARY_OP_INPLACE_ADD_UNICODE#122253
Uh oh!
There was an error while loading.Please reload this page.
Conversation
markshannon left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've suggested a couple of clarifications, but looks good in general.
| *target_local=PyStackRef_FromPyObjectSteal(temp); | ||
| _Py_DECREF_SPECIALIZED(right_o,_PyUnicode_ExactDealloc); | ||
| ERROR_IF(PyStackRef_IsNull(*target_local),error); | ||
| #ifTIER_ONE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you add a comment explaining how this is handled in tier 2.
Python/optimizer.c Outdated
| // Add cache size for opcode | ||
| instr+=_PyOpcode_Caches[_PyOpcode_Deopt[opcode]]; | ||
| if (opcode==BINARY_OP_INPLACE_ADD_UNICODE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you move this up to theif (uop == _BINARY_OP_INPLACE_ADD_UNICODE) clause above?
I'd like to keep the special casing code in one place.
You'll probably want to add an assert this uop is the last in the expansion, as well. Like we do inif (uop == _PUSH_FRAME) above.
Uh oh!
There was an error while loading.Please reload this page.
This one's a bit weird, since it needs the oparg of the following instruction. This is readily available in tier one, but needs to be smuggled in using an operand in tier two.
Technically, it would be cleaner to just add an additional cache entry to all
BINARY_OPinstructions and use that here. However, that feels very wasteful for an instruction that already requires quite a bit of special-casing anyways. So instead, just toss the needed value in the operand during projection.This is mostly for completeness. No impact onperf orstats, although if you squint hard enough you can see very slight increases in trace length and decreases in number of traces and tier one instructions.