gh-93143: Avoid NULL check in LOAD_FAST based on analysis in the compiler #93144
Conversation
|
I collected stats for
In other words, 99.80% of all LOAD_FAST can be converted. |
|
When you're done making the requested changes, leave the comment: |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @ericsnowcurrently: please review the changes made to this pull request. |
|
I'll trust @markshannon and @brandtbucher to review this further. 😄 |
|
Performance looks good, if a bit noisy |
|
Code changes look good to me. |
Python/compile.c
Outdated
| PyObject *index = PyDict_GetItem(c->u->u_varnames, param); | ||
| assert(index != NULL); | ||
| assert(PyLong_CheckExact(index)); | ||
| long l_index = PyLong_AsLong(index); |
There was a problem hiding this comment.
Yep, it looks like it. The assertion passed, so I removed that code.
Looking at symtable.c, It looks like every VISIT(st, arguments, ...); is immediately preceded by a symtable_enter_block(...), so parameters should be guaranteed to be at the lowest indices.
| assert(instr->i_opcode != STORE_FAST__LOAD_FAST); | ||
| assert(instr->i_opcode != LOAD_CONST__LOAD_FAST); | ||
| assert(instr->i_opcode != STORE_FAST__STORE_FAST); | ||
| assert(instr->i_opcode != LOAD_FAST__LOAD_CONST); |
There was a problem hiding this comment.
I recently added IS_ASSEMBLER_OPCODE to contain a list of opcodes that we don't emit in code gen stage, but rather the assembler can use them to replace other opcodes. Would the superinstruction opcodes fit in that list?
There was a problem hiding this comment.
As far as I know, super-instructions are only ever created at quickening time, not compile-time/assembly-time, so I'm not sure it's right to call them assembler opcodes. I think I remember someone hinting at that potentially changing so that all code is quickened by default? If this becomes the case then maybe it makes sense to add an IS_SUPERINSTRUCTION() macro?
sweeneyde
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @sweeneyde for commit d569c5f 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
#93143