gh-115999: Add free-threaded specialization for COMPARE_OP #126410
Conversation
…E_OP specialization in general.
Yhg1s
added
type-feature
| @@ -1335,6 +1335,32 @@ def test_call_specialize(self): | |||
| got = self.get_disassembly(co, adaptive=True) | |||
| self.do_disassembly_compare(got, call_quicken) | |||
|
|
|||
| @cpython_only | |||
There was a problem hiding this comment.
Can we move this test to a more appropriate location, like test_opcache?
Also, can we avoid hardcoding sequences of instructions into the tests and use a behavioral approach: testing that the behavior is the same with and without specialization.
We should already have tests for specialization of COMPARE_OP somewhere.
There was a problem hiding this comment.
Behavioural tests specifically for specialization sound like a great idea regardless of whether we're free-threaded or not. They should still make sure that actual specialization happened, and not just exercise some code. As far as I can tell we don't have any of those right now. I assume that was on purpose.
I agree we should already have tests for specialization of COMPARE_OP, but we don't. That's why I added the new test, similar to the existing test for specialization of BINARY_OP, which Matt adapted for free-threaded specialization in #123926. That test existed here, in test_dis. I assume they were added here on purpose. Should all those tests be moved to a more appropriate location?
There was a problem hiding this comment.
Please take a look: #126498,
even I am not sure Mark intended these kinds of tests.
(If I add COMPARE_OP tests, it is properly failed.)
|
When you're done making the requested changes, leave the comment: |
| @@ -2393,12 +2394,12 @@ _Py_Specialize_CompareOp(_PyStackRef lhs_st, _PyStackRef rhs_st, _Py_CODEUNIT *i | |||
| goto failure; | |||
| } | |||
| if (PyFloat_CheckExact(lhs)) { | |||
| instr->op.code = COMPARE_OP_FLOAT; | |||
| specialized_op = COMPARE_OP_FLOAT; | |||
There was a problem hiding this comment.
Now you can use helpers to write it, like specialize(instr, COMPARE_OR_FLOAT) which looks clearer.
(You need to merge main to get changes from 9ce4fa0)
| goto success; | ||
| } | ||
| } | ||
| SPECIALIZATION_FAIL(COMPARE_OP, compare_op_fail_kind(lhs, rhs)); | ||
| failure: | ||
| STAT_INC(COMPARE_OP, failure); | ||
| instr->op.code = COMPARE_OP; | ||
| SET_OPCODE_OR_RETURN(instr, COMPARE_OP); |
There was a problem hiding this comment.
The same applies to that piece of code, you can remove both of failure and success blocks, now you just need to do unspecialize(instr, compare_op_fail_kind(lhs, rhs)). And in fact there's no more a SET_OPCODE_OR_RETURN macro :)
Also don't forget to remove the #ifdef Py_STATS directive near the compare_op_fail_kind.
Add free-threaded specialization for COMPARE_OP, and tests for COMPARE_OP specialization in general. This relies on thread-local bytecode and atomic operations to update the counters and bytecode, which are single operations.
--disable-gilbuilds #115999