FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-90043: Handle NaNs in `COMPARE_OP_FLOAT_JUMP` by brandtbucher · Pull Request #100278 · python/cpython · GitHub

/ cpython Public

GH-90043: Handle NaNs in COMPARE_OP_FLOAT_JUMP #100278

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

Merged
brandtbucher merged 5 commits into from
Dec 16, 2022
Conversation 14 Commits 5 Checks 0 Files changed
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 .c  (2) .h  (1) .rst  (1) All 3 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2022-12-15-00-50-25.gh-issue-90043.gyoKdx.rst
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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));

sweeneyde Dec 15, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Clever -- roughly the same number of instructions too!

brandtbucher Dec 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

mdickinson Dec 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

mdickinson Dec 16, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

markshannon Dec 16, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mdickinson Dec 16, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

brandtbucher Dec 16, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mdickinson Dec 16, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

mdickinson Dec 16, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

mdickinson Dec 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(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
Show comments
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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;

sweeneyde Dec 15, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

brandtbucher Dec 16, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Back | FazBrowse Home | New Git URL