GH-90043: Handle NaNs in `COMPARE_OP_FLOAT_JUMP` #100278

brandtbucher · 2022-12-15T21:05:24Z

COMPARE_OP_FLOAT_JUMP contains two DEOPT_IF branches for NaN values. This means that COMPARE_OP will repeatedly specialize and deoptimize in the presence of NaNs.

The current scheme can be slightly modified to handle unordered operands correctly, and remove these checks entirely.

Issue: Specialize COMPARE_OP #90043

netlify · 2022-12-15T21:05:28Z

✅ Deploy Preview for python-cpython-preview ready!

Name	Link
🔨 Latest commit	`9dc68aa`
🔍 Latest deploy log	https://app.netlify.com/sites/python-cpython-preview/deploys/639b8c16d9b4720008a2e72e
😎 Deploy Preview	https://deploy-preview-100278--python-cpython-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

sweeneyde · 2022-12-15T23:56:25Z

Python/bytecodes.c

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


Clever -- roughly the same number of instructions too!

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.

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.

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

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

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

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

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.

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.

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

sweeneyde · 2022-12-15T23:58:42Z

Python/specialize.c

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


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.

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.

* main: pythongh-99540: Constant hash for _PyNone_Type to aid reproducibility (pythonGH-99541) pythongh-100039: enhance __signature__ to work with str and callables (pythonGH-100168) pythongh-99830: asyncio: Document returns of remove_{reader,writer} (python#100302) "Compound statement" docs: Fix with-statement step indexing (python#100286) pythonGH-90043: Handle NaNs in COMPARE_OP_FLOAT_JUMP (pythonGH-100278)

* origin/main: (1306 commits) Correct CVE-2020-10735 documentation (python#100306) pythongh-100272: Fix JSON serialization of OrderedDict (pythonGH-100273) pythongh-93649: Split tracemalloc tests from _testcapimodule.c (python#99551) Docs: Use `PY_VERSION_HEX` for version comparison (python#100179) pythongh-97909: Fix markup for `PyMethodDef` members (python#100089) pythongh-99240: Reset pointer to NULL when the pointed memory is freed in argument parsing (python#99890) pythongh-99240: Reset pointer to NULL when the pointed memory is freed in argument parsing (python#99890) pythonGH-98831: Add DECREF_INPUTS(), expanding to DECREF() each stack input (python#100205) pythongh-78707: deprecate passing >1 argument to `PurePath.[is_]relative_to()` (pythonGH-94469) pythongh-99540: Constant hash for _PyNone_Type to aid reproducibility (pythonGH-99541) pythongh-100039: enhance __signature__ to work with str and callables (pythonGH-100168) pythongh-99830: asyncio: Document returns of remove_{reader,writer} (python#100302) "Compound statement" docs: Fix with-statement step indexing (python#100286) pythonGH-90043: Handle NaNs in COMPARE_OP_FLOAT_JUMP (pythonGH-100278) Improve stats presentation for calls. (pythonGH-100274) Better stats for `LOAD_ATTR` and `STORE_ATTR` (pythonGH-100295) pythongh-81057: Move the Cached Parser Dummy Name to _PyRuntimeState (python#100277) Document that zipfile's pwd parameter is a `bytes` object (python#100209) pythongh-99767: mark `PyTypeObject.tp_watched` as internal use only in table (python#100271) Fix typo in introduction.rst (python#100266) ...

brandtbucher added 5 commits November 28, 2022 08:24

Handle NaNs in COMPARE_OP_FLOAT_JUMP

f0728cf

Remove NaN checks!

2665c9a

Catch up with main

dba5894

Catch up with main

66d2d63

blurb add

9dc68aa

brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Dec 15, 2022

brandtbucher requested review from markshannon and sweeneyde December 15, 2022 21:05

brandtbucher self-assigned this Dec 15, 2022

bedevere-bot mentioned this pull request Dec 15, 2022

Specialize COMPARE_OP #90043

Closed

bedevere-bot added the awaiting core review label Dec 15, 2022

sweeneyde approved these changes Dec 16, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Dec 16, 2022

markshannon approved these changes Dec 16, 2022

View reviewed changes

brandtbucher merged commit 9076455 into python:main Dec 16, 2022

bedevere-bot removed the awaiting merge label Dec 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-90043: Handle NaNs in `COMPARE_OP_FLOAT_JUMP` #100278

GH-90043: Handle NaNs in `COMPARE_OP_FLOAT_JUMP` #100278

brandtbucher commented Dec 15, 2022 •

edited by bedevere-bot

Loading

netlify bot commented Dec 15, 2022 •

edited

Loading

sweeneyde Dec 15, 2022

brandtbucher Dec 16, 2022 •

edited

Loading

mdickinson Dec 16, 2022 •

edited

Loading

mdickinson Dec 16, 2022

markshannon Dec 16, 2022

mdickinson Dec 16, 2022

brandtbucher Dec 16, 2022

mdickinson Dec 16, 2022

mdickinson Dec 16, 2022

mdickinson Dec 16, 2022 •

edited

Loading

sweeneyde Dec 15, 2022

brandtbucher Dec 16, 2022

	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)

GH-90043: Handle NaNs in COMPARE_OP_FLOAT_JUMP #100278

GH-90043: Handle NaNs in COMPARE_OP_FLOAT_JUMP #100278

Conversation

brandtbucher commented Dec 15, 2022 • edited by bedevere-bot Loading

netlify bot commented Dec 15, 2022 • edited Loading

✅ Deploy Preview for python-cpython-preview ready!

Choose a reason for hiding this comment

brandtbucher Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

mdickinson Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GH-90043: Handle NaNs in `COMPARE_OP_FLOAT_JUMP` #100278

GH-90043: Handle NaNs in `COMPARE_OP_FLOAT_JUMP` #100278

brandtbucher commented Dec 15, 2022 •

edited by bedevere-bot

Loading

netlify bot commented Dec 15, 2022 •

edited

Loading

brandtbucher Dec 16, 2022 •

edited

Loading

mdickinson Dec 16, 2022 •

edited

Loading

mdickinson Dec 16, 2022 •

edited

Loading