Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-115999: Add free-threaded specialization for COMPARE_OP #126410

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+35 −7
Open

gh-115999: Add free-threaded specialization for COMPARE_OP #126410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7457d45
Add free-threaded specialization for COMPARE_OP, and tests for COMPAR…
Yhg1s Nov 1, 2024
0adf726
Merge branch 'main' into compare_op
Yhg1s Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions Lib/test/test_dis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@corona10 corona10 Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

@requires_specialization_ft
def test_compare_specialize(self):
compare_op_quicken = """\
0 RESUME_CHECK 0

1 LOAD_NAME 0 (a)
LOAD_NAME 1 (b)
%s
RETURN_VALUE
"""
co_int = compile('a == b', "<int>", "eval")
self.code_quicken(lambda: exec(co_int, {}, {'a': 1, 'b': 2}))
got = self.get_disassembly(co_int, adaptive=True)
self.do_disassembly_compare(got, compare_op_quicken % "COMPARE_OP_INT 72 (==)")

co_float = compile('a == b', "<int>", "eval")
self.code_quicken(lambda: exec(co_float, {}, {'a': 1.0, 'b': 2.0}))
got = self.get_disassembly(co_float, adaptive=True)
self.do_disassembly_compare(got, compare_op_quicken % "COMPARE_OP_FLOAT 72 (==)")

co_unicode = compile('a == b', "<unicode>", "eval")
self.code_quicken(lambda: exec(co_unicode, {}, {'a': 'a', 'b': 'b'}))
got = self.get_disassembly(co_unicode, adaptive=True)
self.do_disassembly_compare(got, compare_op_quicken % "COMPARE_OP_STR 72 (==)")

@cpython_only
@requires_specialization
def test_loop_quicken(self):
Expand Down
2 changes: 1 addition & 1 deletion Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2381,7 +2381,7 @@ dummy_func(
};

specializing op(_SPECIALIZE_COMPARE_OP, (counter/1, left, right -- left, right)) {
#if ENABLE_SPECIALIZATION
#if ENABLE_SPECIALIZATION_FT
if (ADAPTIVE_COUNTER_TRIGGERS(counter)) {
next_instr = this_instr;
_Py_Specialize_CompareOp(left, right, next_instr, oparg);
Expand Down
2 changes: 1 addition & 1 deletion Python/generated_cases.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 7 additions & 5 deletions Python/specialize.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2382,8 +2382,9 @@ _Py_Specialize_CompareOp(_PyStackRef lhs_st, _PyStackRef rhs_st, _Py_CODEUNIT *i
{
PyObject *lhs = PyStackRef_AsPyObjectBorrow(lhs_st);
PyObject *rhs = PyStackRef_AsPyObjectBorrow(rhs_st);
uint8_t specialized_op;

assert(ENABLE_SPECIALIZATION);
assert(ENABLE_SPECIALIZATION_FT);
assert(_PyOpcode_Caches[COMPARE_OP] == INLINE_CACHE_ENTRIES_COMPARE_OP);
// All of these specializations compute boolean values, so they're all valid
// regardless of the fifth-lowest oparg bit.
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}
if (PyLong_CheckExact(lhs)) {
if (_PyLong_IsCompact((PyLongObject *)lhs) && _PyLong_IsCompact((PyLongObject *)rhs)) {
instr->op.code = COMPARE_OP_INT;
specialized_op = COMPARE_OP_INT;
goto success;
}
else {
Expand All @@ -2413,18 +2414,19 @@ _Py_Specialize_CompareOp(_PyStackRef lhs_st, _PyStackRef rhs_st, _Py_CODEUNIT *i
goto failure;
}
else {
instr->op.code = COMPARE_OP_STR;
specialized_op = COMPARE_OP_STR;
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cache->counter = adaptive_counter_backoff(cache->counter);
return;
success:
STAT_INC(COMPARE_OP, success);
SET_OPCODE_OR_RETURN(instr, specialized_op);
cache->counter = adaptive_counter_cooldown();
}

Expand Down
Loading