Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

float('nan') returns 0.0 on Python compiled with icc #65366

Closed
hniksic mannequin opened this issue Apr 7, 2014 · 31 comments
Closed

float('nan') returns 0.0 on Python compiled with icc #65366

hniksic mannequin opened this issue Apr 7, 2014 · 31 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@hniksic
Copy link
Mannequin

hniksic mannequin commented Apr 7, 2014

BPO 21167
Nosy @mdickinson, @larryhastings, @hniksic, @bitdancer
Dependencies
  • bpo-21121: -Werror=declaration-after-statement is added even for extension modules through setup.py
  • Files
  • intel-nan.diff: Patch implementing alternative NaN definition under icc
  • intel-nan-safe.patch: Patch for safe NaN with FP optimizations
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2015-08-25.01:15:40.437>
    created_at = <Date 2014-04-07.10:02:32.170>
    labels = ['interpreter-core', 'type-bug']
    title = "float('nan') returns 0.0 on Python compiled with icc"
    updated_at = <Date 2015-08-25.21:35:25.618>
    user = 'https://github.com/hniksic'

    bugs.python.org fields:

    activity = <Date 2015-08-25.21:35:25.618>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-08-25.01:15:40.437>
    closer = 'r.david.murray'
    components = ['Interpreter Core']
    creation = <Date 2014-04-07.10:02:32.170>
    creator = 'hniksic'
    dependencies = ['21121']
    files = ['34746', '40162']
    hgrepos = []
    issue_num = 21167
    keywords = ['patch']
    message_count = 31.0
    messages = ['215687', '215689', '215692', '215698', '215700', '215701', '215703', '215705', '215706', '215707', '216454', '216468', '216583', '216584', '219027', '236244', '236245', '248407', '248410', '248424', '248469', '248478', '248483', '248484', '248519', '248520', '248521', '248549', '248666', '249079', '249164']
    nosy_count = 8.0
    nosy_names = ['mark.dickinson', 'larry', 'hniksic', 'r.david.murray', 'python-dev', 'jbradaric', 'Scholes.C', 'christopher.hogan']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue21167'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5', 'Python 3.6']

    @hniksic
    Copy link
    Mannequin Author

    hniksic mannequin commented Apr 7, 2014

    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.

    @hniksic hniksic mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Apr 7, 2014
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Apr 7, 2014

    Did you compile with "-fp-model strict"? IIRC icc is not IEEE-754
    compliant by default.

    @mdickinson
    Copy link
    Member

    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.

    @hniksic
    Copy link
    Mannequin Author

    hniksic mannequin commented Apr 7, 2014

    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.

    @hniksic
    Copy link
    Mannequin Author

    hniksic mannequin commented Apr 7, 2014

    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.

    @mdickinson
    Copy link
    Member

    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.

    @mdickinson
    Copy link
    Member

    If Python requires strict IEEE 754 floating-point,

    It doesn't (at the moment).

    @hniksic
    Copy link
    Mannequin Author

    hniksic mannequin commented Apr 7, 2014

    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.

    @mdickinson
    Copy link
    Member

    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.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Apr 7, 2014

    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.

    @hniksic
    Copy link
    Mannequin Author

    hniksic mannequin commented Apr 16, 2014

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

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Apr 16, 2014

    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 bpo-21121.

    @mdickinson
    Copy link
    Member

    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.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Apr 16, 2014

    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.

    @hniksic
    Copy link
    Mannequin Author

    hniksic mannequin commented May 24, 2014

    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.

    @ScholesC
    Copy link
    Mannequin

    ScholesC mannequin commented Feb 20, 2015

    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

    @ScholesC
    Copy link
    Mannequin

    ScholesC mannequin commented Feb 20, 2015

    please disregard , the issue is resolved with your patch.

    @christopherhogan
    Copy link
    Mannequin

    christopherhogan mannequin commented Aug 11, 2015

    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.

    @bitdancer
    Copy link
    Member

    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.

    @mdickinson
    Copy link
    Member

    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__?

    @christopherhogan
    Copy link
    Mannequin

    christopherhogan mannequin commented Aug 12, 2015

    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

    @bitdancer
    Copy link
    Member

    I've applied this patch to 2.7 on OSX and compiled without -fp-model strict, and all of the tests now pass.

    @bitdancer
    Copy link
    Member

    Now, what's the equivalent patch for python3? Should I open a new issue for that?

    @bitdancer
    Copy link
    Member

    Nevermind, I forgot to try and see if it applied...and it does :)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 13, 2015

    New changeset d9c85b6bab3a by R David Murray in branch '2.7':
    bpo-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':
    bpo-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: bpo-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: bpo-21167: Fix definition of NAN when ICC used without -fp-model strict.
    https://hg.python.org/cpython/rev/1dd4f473c627

    @bitdancer
    Copy link
    Member

    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.

    @bitdancer
    Copy link
    Member

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

    @larryhastings
    Copy link
    Contributor

    Assuming that "ICC_NAN_STRICT" is only on for Intel icc: yes, please.

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer reopened this Aug 15, 2015
    @larryhastings
    Copy link
    Contributor

    Pull request accepted and merged.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 25, 2015

    New changeset d93bb3d636cf by R David Murray in branch '3.5':
    bpo-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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants