GH-90043: Handle NaNs in COMPARE_OP_FLOAT_JUMP#100278
Uh oh!
There was an error while loading. Please reload this page.
Conversation
brandtbucher added performance 
✅ Deploy Preview for python-cpython-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| 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 | ||
| intsign_ish=1 << (2* (dleft >= dright) + (dleft <= dright)); |
There was a problem hiding this comment.
Clever -- roughly the same number of instructions too!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sorry, that should be islessequal of course, not isless.
There was a problem hiding this comment.
float_richcompare uses simple i < j comparisons. Why is that OK, but not here?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
Lines 600 to 621 in ca079a3
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.
There was a problem hiding this comment.
(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.)
| intwhen_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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) ...
5 participants
COMPARE_OP_FLOAT_JUMPcontains twoDEOPT_IFbranches for NaN values. This means thatCOMPARE_OPwill 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.