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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2015-08-24 23:47 |
Pull request accepted and merged.
|
msg249164 - (view) |
Author: Roundup Robot (python-dev) |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:01 | admin | set | github: 65366 |
2015-08-25 21:35:25 | python-dev | set | messages:
+ msg249164 |
2015-08-25 01:15:40 | r.david.murray | set | status: open -> closed |
2015-08-24 23:47:28 | larry | set | messages:
+ msg249079 |
2015-08-15 22:42:03 | r.david.murray | set | status: closed -> open
messages:
+ msg248666 |
2015-08-14 01:33:37 | larry | set | messages:
+ msg248549 |
2015-08-13 14:22:28 | r.david.murray | set | nosy:
+ larry messages:
+ msg248521
|
2015-08-13 14:12:12 | r.david.murray | set | status: open -> closed resolution: fixed messages:
+ msg248520
stage: resolved |
2015-08-13 14:09:51 | python-dev | set | nosy:
+ python-dev messages:
+ msg248519
|
2015-08-12 23:04:27 | r.david.murray | set | messages:
+ msg248484 versions:
+ Python 3.4, Python 3.5, Python 3.6 |
2015-08-12 23:00:27 | r.david.murray | set | messages:
+ msg248483 |
2015-08-12 21:56:41 | r.david.murray | set | messages:
+ msg248478 |
2015-08-12 18:12:32 | christopher.hogan | set | messages:
+ msg248469 |
2015-08-11 19:04:06 | mark.dickinson | set | messages:
+ msg248424 |
2015-08-11 15:17:06 | r.david.murray | set | nosy:
+ r.david.murray messages:
+ msg248410
|
2015-08-11 14:51:22 | christopher.hogan | set | files:
+ intel-nan-safe.patch nosy:
+ christopher.hogan messages:
+ msg248407
|
2015-02-20 00:26:28 | Scholes.C | set | messages:
+ msg236245 |
2015-02-20 00:17:42 | Scholes.C | set | nosy:
+ Scholes.C messages:
+ msg236244
|
2015-02-19 09:19:28 | SilentGhost | link | issue23483 superseder |
2014-10-14 18:03:41 | skrah | set | nosy:
- skrah
|
2014-05-24 07:22:34 | hniksic | set | messages:
+ msg219027 |
2014-04-16 20:43:55 | skrah | set | messages:
+ msg216584 |
2014-04-16 20:31:42 | mark.dickinson | set | messages:
+ msg216583 |
2014-04-16 14:06:53 | skrah | set | dependencies:
+ -Werror=declaration-after-statement is added even for extension modules through setup.py messages:
+ msg216468 |
2014-04-16 09:20:13 | hniksic | set | messages:
+ msg216454 |
2014-04-07 12:24:29 | skrah | set | messages:
+ msg215707 |
2014-04-07 11:47:48 | mark.dickinson | set | messages:
+ msg215706 |
2014-04-07 11:40:31 | hniksic | set | messages:
+ msg215705 |
2014-04-07 11:35:05 | mark.dickinson | set | versions:
- Python 3.5 |
2014-04-07 11:31:40 | mark.dickinson | set | messages:
+ msg215703 |
2014-04-07 11:30:51 | mark.dickinson | set | messages:
+ msg215701 |
2014-04-07 11:27:50 | hniksic | set | messages:
+ msg215700 |
2014-04-07 11:25:31 | hniksic | set | messages:
+ msg215698 |
2014-04-07 11:04:39 | mark.dickinson | set | messages:
+ msg215692 |
2014-04-07 10:40:35 | jbradaric | set | nosy:
+ jbradaric
|
2014-04-07 10:11:05 | skrah | set | nosy:
+ skrah messages:
+ msg215689
|
2014-04-07 10:03:36 | vstinner | set | nosy:
+ mark.dickinson
|
2014-04-07 10:02:32 | hniksic | create | |