classification
Title: float('nan') returns 0.0 on Python compiled with icc
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.6, Python 3.4, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: 21121 Superseder:
Assigned To: Nosy List: Scholes.C, christopher.hogan, hniksic, jbradaric, larry, mark.dickinson, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2014-04-07 10:02 by hniksic, last changed 2015-08-25 21:35 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
intel-nan.diff hniksic, 2014-04-07 10:02 Patch implementing alternative NaN definition under icc
intel-nan-safe.patch christopher.hogan, 2015-08-11 14:51 Patch for safe NaN with FP optimizations review
Messages (31)
msg215687 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2014-04-07 10:02
On a Python compiled with Intel C compiler, float('nan') returns 0.0. This behavior can be reproduced with icc versions 11 and 12.

The definition of Py_NAN in include/pymath.h expects `HUGE_VAL * 0.0` to compile to a NaN value on IEEE754 platforms:

/* Py_NAN
 * A value that evaluates to a NaN. On IEEE 754 platforms INF*0 or
 * INF/INF works. Define Py_NO_NAN in pyconfig.h if your platform
 * doesn't support NaNs.
 */
#if !defined(Py_NAN) && !defined(Py_NO_NAN)
#define Py_NAN (Py_HUGE_VAL * 0.)
#endif

I don't have a copy of the standard, but a simple test shows that the above does not hold for icc. When compiled with icc without any flags, the following program outputs "inf 0.000000" instead of the expected "inf nan":

#include <stdio.h>
#include <math.h>

int main()
{
  double inf = HUGE_VAL;
  double nan = HUGE_VAL * 0;
  printf("%f %f\n", inf, nan);
  return 0;
}

Compiling the same program with gcc yields the expected output of "inf nan".

This issue can be fixed (or worked around, if it's an icc bug) by using a different definition of Py_NAN. On icc, defining Py_NAN as `Py_HUGE_VAL / Py_HUGE_VAL` produces the intended effect. If this is considered suboptimal on other compilers, the definition can be conditionally compiled for icc only.

The attached patch fixes the issue, taking the conservative approach of only modifying the icc definition of Py_NAN.
msg215689 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-04-07 10:11
Did you compile with "-fp-model strict"? IIRC icc is not IEEE-754
compliant by default.
msg215692 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2014-04-07 11:04
What's `sys.float_repr_style` for this build?  For Python 3.5 and short float repr, `float('nan')` shouldn't even be using Py_NAN.  It should land in _Py_parse_inf_or_nan in pystrtod.c, which uses `_Py_dg_stdnan` to get the NaN value directly from a suitable bitpattern.
msg215698 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2014-04-07 11:25
sys.float_repr_style is 'short'.

I haven't actually tried this in Python 3.5, but I did note the same definition of Py_NAN (which is used elsewhere in the code), so I tagged the issue as likely also present in 3.x.
msg215700 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2014-04-07 11:27
The compilation was performed with the default flags, i.e. without -fp-model strict or similar.

If Python requires strict IEEE 754 floating-point, it should probably be mentioned at a prominent place in the documentation. Administrators building Python with alternative compilers or on non-gcc Unix systems might not be aware of the issue.
msg215701 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2014-04-07 11:30
> I tagged the issue as likely also present in 3.x.

So I believe that this particular issue (float('nan') giving zero) should not be present on Python 3.4 or above.  The bogus definition of Py_NAN will still cause problems in some math and cmath return values, though.
msg215703 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2014-04-07 11:31
> If Python requires strict IEEE 754 floating-point,

It doesn't (at the moment).
msg215705 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2014-04-07 11:40
Mark:
> > If Python requires strict IEEE 754 floating-point,
> 
> It doesn't (at the moment).

Does this imply that my patch is a better fix than requiring the builder to specify -fp-model strict to icc?

For our use case either solution is valid. For administrators and users testing or using Python with icc, it might be more convenient for Py_NAN to do the right thing with the default icc flags.
msg215706 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2014-04-07 11:47
> It doesn't (at the moment).

Sorry; that was an unhelpful kneejerk reply.

There are two aspects to this. (1) Currently, at least in theory, CPython doesn't require IEEE 754 *at all*:  in theory, it should work with whatever floating-point format the platform's "double" type happens to provide.  In practice I suspect there's a lot more IEEE 754 dependence in the codebase than we realize, and I'd expect to see a fair amount of test breaking if Python were ever to meet a non-IEEE 754 platform.  (Reports of any *recent* occurrences of Python in the wild meeting non-IEEE 754 platforms would be welcomed.)

(2) If CPython *does* figure out that it's running on an IEEE 754 platform then yes, care should be taken with compiler flags to ensure that we're not using compiler optimisations that have the potential to break IEEE semantics.  Without this there's a significant danger of float to string conversions getting messed up;  there are also a few odds and ends in the math and cmath libraries that depend on careful IEEE semantics.  (The implementations of fsum and log1p, for example.)

I think the right fix here is to add the relevant compiler flag to the build.  That said, I'd *also* love to see remaining uses of Py_NAN fixed to use _Py_dg_stdnan where available.
msg215707 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-04-07 12:24
I agree we should add "-fp-model strict" to the icc build flags, provided that
there is a way that it does not show up in the distutils flags.

Apparently icc users want unsafe fast math by default, so this flag should
probably not be enforced in extension module builds.
msg216454 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2014-04-16 09:20
Using -fp-model strict (or other appropriate icc flag) seems like a reasonable resolution.

It should likely also be applied to Python 3.x, despite the version field of this issue. (Even if float('nan') happens to work in current 3.x, internal code that returns Py_NAN can and will break.)
msg216468 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-04-16 14:06
Mark, if you agree that "fp-model strict" should not show up in
the distutils CFLAGS once Python is installed, the issue now depends
on #21121.
msg216583 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2014-04-16 20:31
Sure, if that's really the expectation.  I'm not sure I'd want any of *my* extension modules being built with non-strict FP, but there's a bit of a personal bias there.
msg216584 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-04-16 20:43
I quite agree, and it's hard to tell what users want. Basically I'm afraid
of a bug report "C extension using unsafe math got slower in Python-3.5".

I would find either decision reasonable.
msg219027 - (view) Author: Hrvoje Nikšić (hniksic) * Date: 2014-05-24 07:22
Note that defaulting to unsafe math in extensions will make *their* use of the Py_NAN macro break under icc.

If we go that route ("-fp-model strict" for Python build, but not for extensions), we should also apply the attached patch that defines Py_NAN as Py_HUGE_VAL / Py_HUGE_VAL.
msg236244 - (view) Author: Scholes C (Scholes.C) Date: 2015-02-20 00:17
HI,
can you please look into this ?
thanks.

icc builds of python 2.7 seem to have issues handling nan, inf, etc

$ /usr/local/python-2.7.6/bin/python
Python 2.7.6 (default, Jan 10 2014, 12:14:02)
[GCC Intel(R) C++ gcc 4.1 mode] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> print float('nan')
0.0


$ /usr/local/python-2.6.6-64bit/bin/python
Python 2.6.6 (r266:84292, Oct 14 2010, 15:47:19)
[GCC Intel(R) C++ gcc 4.1 mode] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> print float('nan')
nan


I tried both –fp-model strict and –fp-model precise compiler options as suggested by http://bugs.python.org/issue21167, but neither seems to resolve other situations like the one with atan2 below:

$ LD_LIBRARY_PATH=/dat/sharefolder_scratch/python-build ./python
Python 2.7.9 (default, Feb 18 2015, 19:58:37)
[GCC Intel(R) C++ gcc 4.1 mode] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> print float('nan')
nan
>>> import math
>>> print math.atan2(0, float('nan'))
0.0

$ /usr/local/python-2.6.6-64bit/bin/python
Python 2.6.6 (r266:84292, Oct 14 2010, 15:47:19)
[GCC Intel(R) C++ gcc 4.1 mode] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import math
>>> print math.atan2(0, float('nan'))
nan
msg236245 - (view) Author: Scholes C (Scholes.C) Date: 2015-02-20 00:26
please disregard , the issue is resolved with your patch.
msg248407 - (view) Author: Chris Hogan (christopher.hogan) * Date: 2015-08-11 14:51
Producing NaN by Py_HUGE_VAL / Py_HUGE_VAL as in the suggested patch is unsafe as it can generate a FP exception during runtime. Also aggresive compiler FP optimizations can eliminate this calculation on compile-time. For this reason, we've used constant referencing in our fix, which will work regardless of how -fp-model is set.

The problem is that the compiler is free to pre-compute the result as both values in 0*Inf are constants. An aggressively optimizing compiler could treat 0 * x = 0 no matter what x is. So under aggressive floating point optimizations setting we could get a wrong value, and that is indeed what happens. 

Another problem is that 0 * Inf along with resulting in a QNaN value should raise an invalid floating point exception flag. If the result is pre-computed at compile time, then the user won’t see the flag (it is another question whether the user wanted the flag or not originally).

Our patch preserves both the value and the side effect, and leaves people free to build with the flags they want.
msg248410 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-11 15:17
Note that Chris' patch is coming from Intel.  (The ICC buildbots are currently building with -fp-model strict, by the way.)

Mark, Stefan, what do you think?  Is this a good idea?  IIUC we would then not have to worry about differentiating between the python build and the distutils flags, and users would get whatever they specified at python build time.
msg248424 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2015-08-11 19:04
Looks fine to me.  IIRC, we moved the PyFloat_FromString implementation away from using Py_NAN in Python 3 for exactly this reason.

On this point, though:

> An aggressively optimizing compiler could treat 0 * x = 0 no matter what x is.

Wouldn't such a compiler be in violation of the C standard, at least if it defines __STDC_IEC_559__?
msg248469 - (view) Author: Chris Hogan (christopher.hogan) * Date: 2015-08-12 18:12
From Clark Nelson:

> In my opinion, exactly how and where the macro is defined that indicates our conformance to the FP standard
> doesn't really matter. The point is that it is our intention to conform, at least to some degree and under 
> some circumstances.
>
> Under those circumstances, it would be wrong to map (0 * x) to 0 without regard to what x might be.
>
> Clark
msg248478 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-12 21:56
I've applied this patch to 2.7 on OSX and compiled without -fp-model strict, and all of the tests now pass.
msg248483 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-12 23:00
Now, what's the equivalent patch for python3?  Should I open a new issue for that?
msg248484 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-12 23:04
Nevermind, I forgot to try and see if it applied...and it does :)
msg248519 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-08-13 14:09
New changeset d9c85b6bab3a by R David Murray in branch '2.7':
#21167: Fix definition of NAN when ICC used without -fp-model strict.
https://hg.python.org/cpython/rev/d9c85b6bab3a

New changeset 5e71a489f01d by R David Murray in branch '3.4':
#21167: Fix definition of NAN when ICC used without -fp-model strict.
https://hg.python.org/cpython/rev/5e71a489f01d

New changeset e3008318f76b by R David Murray in branch '3.5':
Merge: #21167: Fix definition of NAN when ICC used without -fp-model strict.
https://hg.python.org/cpython/rev/e3008318f76b

New changeset 1dd4f473c627 by R David Murray in branch 'default':
Merge: #21167: Fix definition of NAN when ICC used without -fp-model strict.
https://hg.python.org/cpython/rev/1dd4f473c627
msg248520 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-13 14:12
Thanks Chris, and Mark.

I ran the tests on 3.6 both on Linux (non ICC) and on Mac (with ICC without -fp-model strict) and all the tests passed.
msg248521 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-13 14:22
Larry, do you want this for 3.5.0a2?  It's an innocuous patch for anyone not using ICC, and makes ICC "just work" (with the default ICC build arguments) for people using ICC.  (Well, on (lin/u)nux and mac, anyway, I'm not sure we've resolved all the ICC issues on Windows yet.)
msg248549 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-08-14 01:33
Assuming that "ICC_NAN_STRICT" is only on for Intel icc: yes, please.
msg248666 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-15 22:42
Here's the pull request:

https://bitbucket.org/larry/cpython350/pull-requests/4/21167-fix-definition-of-nan-when-icc-used/diff
msg249079 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-08-24 23:47
Pull request accepted and merged.
msg249164 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-08-25 21:35
New changeset d93bb3d636cf by R David Murray in branch '3.5':
#21167: Fix definition of NAN when ICC used without -fp-model strict.
https://hg.python.org/cpython/rev/d93bb3d636cf

New changeset c1523d5dee3c by Larry Hastings in branch '3.5':
Merged in bitdancer/cpython350 (pull request #4)
https://hg.python.org/cpython/rev/c1523d5dee3c
History
Date User Action Args
2015-08-25 21:35:25python-devsetmessages: + msg249164
2015-08-25 01:15:40r.david.murraysetstatus: open -> closed
2015-08-24 23:47:28larrysetmessages: + msg249079
2015-08-15 22:42:03r.david.murraysetstatus: closed -> open

messages: + msg248666
2015-08-14 01:33:37larrysetmessages: + msg248549
2015-08-13 14:22:28r.david.murraysetnosy: + larry
messages: + msg248521
2015-08-13 14:12:12r.david.murraysetstatus: open -> closed
resolution: fixed
messages: + msg248520

stage: resolved
2015-08-13 14:09:51python-devsetnosy: + python-dev
messages: + msg248519
2015-08-12 23:04:27r.david.murraysetmessages: + msg248484
versions: + Python 3.4, Python 3.5, Python 3.6
2015-08-12 23:00:27r.david.murraysetmessages: + msg248483
2015-08-12 21:56:41r.david.murraysetmessages: + msg248478
2015-08-12 18:12:32christopher.hogansetmessages: + msg248469
2015-08-11 19:04:06mark.dickinsonsetmessages: + msg248424
2015-08-11 15:17:06r.david.murraysetnosy: + r.david.murray
messages: + msg248410
2015-08-11 14:51:22christopher.hogansetfiles: + intel-nan-safe.patch
nosy: + christopher.hogan
messages: + msg248407

2015-02-20 00:26:28Scholes.Csetmessages: + msg236245
2015-02-20 00:17:42Scholes.Csetnosy: + Scholes.C
messages: + msg236244
2015-02-19 09:19:28SilentGhostlinkissue23483 superseder
2014-10-14 18:03:41skrahsetnosy: - skrah
2014-05-24 07:22:34hniksicsetmessages: + msg219027
2014-04-16 20:43:55skrahsetmessages: + msg216584
2014-04-16 20:31:42mark.dickinsonsetmessages: + msg216583
2014-04-16 14:06:53skrahsetdependencies: + -Werror=declaration-after-statement is added even for extension modules through setup.py
messages: + msg216468
2014-04-16 09:20:13hniksicsetmessages: + msg216454
2014-04-07 12:24:29skrahsetmessages: + msg215707
2014-04-07 11:47:48mark.dickinsonsetmessages: + msg215706
2014-04-07 11:40:31hniksicsetmessages: + msg215705
2014-04-07 11:35:05mark.dickinsonsetversions: - Python 3.5
2014-04-07 11:31:40mark.dickinsonsetmessages: + msg215703
2014-04-07 11:30:51mark.dickinsonsetmessages: + msg215701
2014-04-07 11:27:50hniksicsetmessages: + msg215700
2014-04-07 11:25:31hniksicsetmessages: + msg215698
2014-04-07 11:04:39mark.dickinsonsetmessages: + msg215692
2014-04-07 10:40:35jbradaricsetnosy: + jbradaric
2014-04-07 10:11:05skrahsetnosy: + skrah
messages: + msg215689
2014-04-07 10:03:36vstinnersetnosy: + mark.dickinson
2014-04-07 10:02:32hniksiccreate