Sorry, something went wrong.
✅ Deploy Preview for python-cpython-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Sorry, something went wrong.
| 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)); |
There was a problem hiding this comment.
Clever -- roughly the same number of instructions too!
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, that should be islessequal of course, not isless.
Sorry, something went wrong.
There was a problem hiding this comment.
float_richcompare uses simple i < j comparisons. Why is that OK, but not here?
Sorry, something went wrong.
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.
Sorry, something went wrong.
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).
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.)
Sorry, something went wrong.
| 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; |
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.