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

GH-90043: Handle NaNs in COMPARE_OP_FLOAT_JUMP #100278

Merged
merged 5 commits into from
Dec 16, 2022
Merged

GH-90043: Handle NaNs in COMPARE_OP_FLOAT_JUMP #100278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f0728cf
Handle NaNs in COMPARE_OP_FLOAT_JUMP
brandtbucher Nov 28, 2022
2665c9a
Remove NaN checks!
brandtbucher Nov 30, 2022
dba5894
Catch up with main
brandtbucher Nov 30, 2022
66d2d63
Catch up with main
brandtbucher Dec 15, 2022
9dc68aa
blurb add
brandtbucher Dec 15, 2022
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
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2022-12-15-00-50-25.gh-issue-90043.gyoKdx.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Handle NaNs when specializing :opcode:`COMPARE_OP` for :class:`float`
values.
12 changes: 5 additions & 7 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2024,13 +2024,11 @@ dummy_func(
// Combined: COMPARE_OP (float ? float) + POP_JUMP_IF_(true/false)
DEOPT_IF(!PyFloat_CheckExact(left), COMPARE_OP);
DEOPT_IF(!PyFloat_CheckExact(right), COMPARE_OP);
STAT_INC(COMPARE_OP, hit);
double dleft = PyFloat_AS_DOUBLE(left);
double dright = PyFloat_AS_DOUBLE(right);
// 1 if <, 2 if ==, 4 if >; this matches when _to_jump_mask
int sign_ish = 2*(dleft > dright) + 2 - (dleft < dright);
DEOPT_IF(isnan(dleft), COMPARE_OP);
DEOPT_IF(isnan(dright), COMPARE_OP);
STAT_INC(COMPARE_OP, hit);
// 1 if NaN, 2 if <, 4 if >, 8 if ==; this matches when_to_jump_mask
int sign_ish = 1 << (2 * (dleft >= dright) + (dleft <= dright));
Copy link
Member

Choose a reason for hiding this comment

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

Clever -- roughly the same number of instructions too!

Copy link
Member Author

@brandtbucher brandtbucher Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Credit to @markshannon for figuring this out. I had tried something similar (with >= and <=), but couldn't get the masks to work correctly without his help.

Copy link
Member

@mdickinson mdickinson Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is potentially a bit dangerous: a comparison with NaN may raise FE_INVALID. It may be safer to use the isless macro instead.

From C99 (§7.12.14.3):

The isless macro determines whether its first argument is less than its second argument. The value of isless(x, y) is always equal to (x) < (y); however, unlike (x) < (y), isless(x, y) does not raise the ‘‘invalid’’ floating-point exception when x and y are unordered.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, that should be islessequal of course, not isless.

Copy link
Member

Choose a reason for hiding this comment

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

float_richcompare uses simple i < j comparisons. Why is that OK, but not here?

Copy link
Member

Choose a reason for hiding this comment

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

@brandtbucher No, not specific to signalling NaNs, just to comparisons involving NaNs. And yes, float_richcompare should be updated to use the safer code, now that we're allowed to depend on C99 features.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. I'll merge this, and create a separate issue where we can discuss how/if we should avoid potential floating-point exceptions everywhere (since that seems like a much bigger change).

Copy link
Member

Choose a reason for hiding this comment

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

And yes, it's platform-specific. I think we're in a place now where all current platforms do the "right" thing, but that wasn't always the case. There were a few related issues in the past - #81655 is the only one I'm finding right now, though. So yes, probably a non-issue at this point.

Copy link
Member

Choose a reason for hiding this comment

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

create a separate issue

Thanks. Don't worry about this, though - I think I'm fussing about things that were historically an issue, but are no longer. It's not that long ago that we did protect all floating-point operations, e.g., here's float_richcompare in Python 2.7:

cpython/Objects/floatobject.c

Lines 600 to 621 in ca079a3

PyFPE_START_PROTECT("richcompare", return NULL)
switch (op) {
case Py_EQ:
r = i == j;
break;
case Py_NE:
r = i != j;
break;
case Py_LE:
r = i <= j;
break;
case Py_GE:
r = i >= j;
break;
case Py_LT:
r = i < j;
break;
case Py_GT:
r = i > j;
break;
}
PyFPE_END_PROTECT(r)

But it seems that all current platforms do follow the IEEE 754 standard's advice on "default" floating-point exception handling settings, so this is something we no longer need to worry about.

Copy link
Member

@mdickinson mdickinson Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

(FTR, it looks like Python 3.9 is where we discarded the PyFPE_START_PROTECT / PyFPE_END_PROTECT guards everywhere. They're still present in 3.8.)

_Py_DECREF_SPECIALIZED(left, _PyFloat_ExactDealloc);
_Py_DECREF_SPECIALIZED(right, _PyFloat_ExactDealloc);
jump = sign_ish & when_to_jump_mask;
Expand All @@ -2057,8 +2055,8 @@ dummy_func(
assert(Py_ABS(Py_SIZE(left)) <= 1 && Py_ABS(Py_SIZE(right)) <= 1);
Py_ssize_t ileft = Py_SIZE(left) * ((PyLongObject *)left)->ob_digit[0];
Py_ssize_t iright = Py_SIZE(right) * ((PyLongObject *)right)->ob_digit[0];
// 1 if <, 2 if ==, 4 if >; this matches when _to_jump_mask
int sign_ish = 2*(ileft > iright) + 2 - (ileft < iright);
// 2 if <, 4 if >, 8 if ==; this matches when_to_jump_mask
int sign_ish = 1 << (2 * (ileft >= iright) + (ileft <= iright));
_Py_DECREF_SPECIALIZED(left, (destructor)PyObject_Free);
_Py_DECREF_SPECIALIZED(right, (destructor)PyObject_Free);
jump = sign_ish & when_to_jump_mask;
Expand Down
12 changes: 5 additions & 7 deletions 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.

21 changes: 11 additions & 10 deletions Python/specialize.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1938,15 +1938,16 @@ compare_op_fail_kind(PyObject *lhs, PyObject *rhs)


static int compare_masks[] = {
// 1-bit: jump if less than
// 2-bit: jump if equal
// 1-bit: jump if unordered
// 2-bit: jump if less
// 4-bit: jump if greater
[Py_LT] = 1 | 0 | 0,
[Py_LE] = 1 | 2 | 0,
[Py_EQ] = 0 | 2 | 0,
[Py_NE] = 1 | 0 | 4,
[Py_GT] = 0 | 0 | 4,
[Py_GE] = 0 | 2 | 4,
// 8-bit: jump if equal
[Py_LT] = 0 | 2 | 0 | 0,
[Py_LE] = 0 | 2 | 0 | 8,
[Py_EQ] = 0 | 0 | 0 | 8,
[Py_NE] = 1 | 2 | 4 | 0,
[Py_GT] = 0 | 0 | 4 | 0,
[Py_GE] = 0 | 0 | 4 | 8,
};

void
Expand All @@ -1967,7 +1968,7 @@ _Py_Specialize_CompareOp(PyObject *lhs, PyObject *rhs, _Py_CODEUNIT *instr,
assert(oparg <= Py_GE);
int when_to_jump_mask = compare_masks[oparg];
if (next_opcode == POP_JUMP_IF_FALSE) {
when_to_jump_mask = (1 | 2 | 4) & ~when_to_jump_mask;
when_to_jump_mask = (1 | 2 | 4 | 8) & ~when_to_jump_mask;
Copy link
Member

Choose a reason for hiding this comment

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

I was skeptical of this at first, since I had thought about this as "use _Py_SwappedOp[op]", which would be wrong. But this looks correct -- it only swaps which branch to take.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. The swapped ops used to be each others' bitwise inversions. That's no longer the case for the "unordered" bit, since NaNs only care if the operation is != or not.

}
if (Py_TYPE(lhs) != Py_TYPE(rhs)) {
SPECIALIZATION_FAIL(COMPARE_OP, compare_op_fail_kind(lhs, rhs));
Expand Down Expand Up @@ -1996,7 +1997,7 @@ _Py_Specialize_CompareOp(PyObject *lhs, PyObject *rhs, _Py_CODEUNIT *instr,
}
else {
_py_set_opcode(instr, COMPARE_OP_STR_JUMP);
cache->mask = (when_to_jump_mask & 2) == 0;
cache->mask = (when_to_jump_mask & 8) == 0;
goto success;
}
}
Expand Down