Title: ICC compiler: ICC treats denormal floating point numbers as 0.0
Type: crash Stage:
Components: Build, Windows Versions: Python 3.6, Python 3.5, Python 2.7
Status: open Resolution:
Dependencies: Superseder:
Assigned To: zach.ware Nosy List: mark.dickinson, paul.moore, pitrou, python-dev, r.david.murray, serhiy.storchaka, skrah, steve.dower, tim.golden, tim.peters, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2015-12-23 23:02 by skrah, last changed 2016-03-18 16:58 by zach.ware.

File name Uploaded Description Edit
issue25934.diff zach.ware, 2016-01-23 05:44 review
Messages (11)
msg256943 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-12-23 23:02
Zachary in #24999:

"If that's the case, would anyone (in particular, Steve, Tim or Tim) mind if we just made the default (for MSVC as well as ICC) /fp:strict?  It would be much easier to just change the global default than to try to either make it settable or to change it just when ICC is used."
msg258631 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2016-01-19 22:50
I've looked into this a bit, and tests won't pass with MSVC /fp:strict, so it would need to be ICC specific.  UNIX ICC builds do unconditionally use -fp-model strict now, though, so I think Windows ICC builds should as well.
msg258652 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-01-20 08:41
Which tests fail with MSVC /fp:strict? It's surprising that making behaviour stricter causes tests to fail that wouldn't have done otherwise. :-)
msg258685 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2016-01-20 15:39
Ah, it's been a while since I tested that, so my reporting was inaccurate.  The problem is actually that the compile fails:

..\Modules\mathmodule.c(1924): error C2099: initializer is not a constant [C:\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\mathmodule.c(1925): error C2099: initializer is not a constant [C:\cpython\PCbuild\pythoncore.vcxproj]

..\Python\dtoa.c(1215): error C2099: initializer is not a constant [C:\cpython\PCbuild\pythoncore.vcxproj]
msg258686 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-01-20 15:52
Thanks. The offending lines in the math module are:

static const double degToRad = Py_MATH_PI / 180.0;
static const double radToDeg = 180.0 / Py_MATH_PI;

It would be trivial to replace these with suitable constants.
msg258687 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-01-20 15:54
I'm -0 on changing the universal default (and want to call out that it's a relevant porting note for 3.6, not appropriate for 3.5), but we can presumably fix those in case someone wants to use strict for their own build.
msg258689 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-01-20 16:07
The dtoa.c occurrence is also straightforward to fix. It's coming from this declaration:

    static const double tinytens[] = { 1e-16, 1e-32, 1e-64, 1e-128,

We need to be a tiny bit careful here, since dtoa.c is fragile code and relies on exact representation of some floats. But this isn't one of them: the only thing that tinytens is used for is getting a first approximation to the correct strtod conversion before the main iteration kicks in.

So replacing that last tinytens value with a suitably precise constant should be okay. The *exact* value of the constant we need is 0x1.8062864ac6f43p-745, or in decimal: 


But 8.112963841460668e-225 should be good enough (along with a comment explaining why the expression was changed). If MSVC supported C99's hex constants, we could just use 0x1.8062864ac6f43p-745. But it doesn't. :-(
msg258853 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2016-01-23 05:44
Here's a patch that specifies /fp:strict if it looks like ICC is being used for the build.  It also adds a convenient property for checking whether it's an ICC build ($(ICCBuild)).  This doesn't change anything for MSVC builds but allows ICC builds to pass test_math, test_cmath, test_audioop, etc.; as such, it should be applied to all three branches.

As far as replacing the non-constants that MSVC doesn't like, I'm +1 but not too bothered about it.  I'm happy to test if anyone supplies a patch for it.
msg259227 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-01-30 01:12
New changeset 296fb7c10a7d by Zachary Ware in branch '2.7':
Issue #25934: Default to /fp:strict for ICC builds

New changeset 747b415e96c4 by Zachary Ware in branch '3.5':
Issue #25934: Default to /fp:strict for ICC builds

New changeset c080ef5989f7 by Zachary Ware in branch 'default':
Issue #25934: Merge with 3.5
msg259337 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2016-02-01 17:52
With the buildbots not angry over this, I'm closing the issue.  I opened #26262 to follow the possibility of changing the initializers to allow /fp:strict with MSVC.
msg261976 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2016-03-18 16:58
I closed this prematurely: I successfully added /fp:strict to ICC builds, but it didn't fix the underlying issue.
Date User Action Args
2016-03-18 16:58:02zach.waresetstatus: closed -> open
resolution: fixed ->
messages: + msg261976

stage: resolved ->
2016-02-01 17:52:08zach.waresetstatus: open -> closed
messages: + msg259337

assignee: zach.ware
resolution: fixed
stage: resolved
2016-01-30 01:12:02python-devsetmessages: + msg259227
2016-01-23 05:44:35zach.waresetfiles: + issue25934.diff
keywords: + patch
messages: + msg258853

versions: + Python 2.7
2016-01-20 16:07:03mark.dickinsonsetmessages: + msg258689
2016-01-20 15:54:49steve.dowersetmessages: + msg258687
2016-01-20 15:52:03mark.dickinsonsetmessages: + msg258686
2016-01-20 15:39:45zach.waresetmessages: + msg258685
2016-01-20 08:41:38mark.dickinsonsetmessages: + msg258652
2016-01-19 22:50:06zach.waresetmessages: + msg258631
2015-12-24 06:32:54serhiy.storchakasettype: crash
components: + Build, Windows
versions: + Python 3.5, Python 3.6
2015-12-23 23:02:23skrahcreate