| msg256943 - (view) |
Author: Stefan Krah (skrah) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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, 9007199254740992.*9007199254740992.e-256}; 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: 8.11296384146066798564414657510725945755617084762542409283393077939218873123696010224953185884350111344494845510586594487062499326481633636403965073720792027659736980514301906428513324403012698167973428740345918815412673646248939187125560378562250718330485204443480974365425682653185460026731686712157238961539250106472701178830382576220700577401565213825995547382545061286464557591871221948351734365233811730381171156150018612038234137396963100802781483162471536048255738998621259419238986828005847002315384502722411971989039148624688058131226853220141492784023284912109375E-225 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) * |
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)  |
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 https://hg.python.org/cpython/rev/296fb7c10a7d New changeset 747b415e96c4 by Zachary Ware in branch '3.5': Issue #25934: Default to /fp:strict for ICC builds https://hg.python.org/cpython/rev/747b415e96c4 New changeset c080ef5989f7 by Zachary Ware in branch 'default': Issue #25934: Merge with 3.5 https://hg.python.org/cpython/rev/c080ef5989f7 |
|
|
| msg259337 - (view) |
Author: Zachary Ware (zach.ware) * |
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) * |
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. |
|
|
| msg404221 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2021-10-18 20:53 |
| I no longer have access to ICC, and the ICC buildbots have been mothballed some years ago. Closing this as out of date; the issue might not be, but the information about it here probably is :) |
|
|
| msg404315 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2021-10-19 16:45 |
| > Closing this as out of date. SGTM. Thanks. |
|
|