This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: ICC on Windows 8.1: _decimal fails to compile with default fp model
Type: compile error Stage: resolved
Components: Build, Windows Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: zach.ware Nosy List: christopher.hogan, mark.dickinson, paul.moore, python-dev, r.david.murray, rhettinger, skrah, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2015-08-31 21:37 by zach.ware, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
_decimal_fp_precise_pragma.diff zach.ware, 2015-09-03 03:58 review
Messages (16)
msg249428 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-08-31 21:37
When building on Windows 8.1 with ICC 15.0 (backed by VS 2015 Community), the _decimal module fails to compile:

ClCompile:
  mpdecimal.c
..\Modules\_decimal\libmpdec\mpdecimal.c(46): error : fenv_access cannot be enabled except in precise, source, double, and extended modes [P:\ath\to\cpython\PCbuild\_decimal.vcxproj]
        #pragma fenv_access(on)
                               ^

Unfortunately, I can't reproduce this on two other machines both running Windows Server 2012 R2, with otherwise nearly identical configurations to mine.

Since I can't reproduce it elsewhere (including on the ICC Windows buildbot) and have a relatively easy workaround (passing '/fp:strict' makes the problem go away), setting priority to 'low' until somebody else can reproduce it.
msg249434 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-08-31 22:12
It should be defaulting to precise mode, but maybe ICC has a different default?

I wrote on #24973:

It's not a safe 3.5 change now, but compiling with /fp:fast by default and using #pragma float_control(precise, push)/#pragma float_control(pop) around that section may be an option for 3.6, if the benchmarks show some value in it. Either way, the pragma is probably the better way to require particular behaviour for that section of code, rather than changing a global option.
msg249476 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-09-01 11:51
On Linux ICC has something like "fast-math" by default.  The situation
is a bit annoying: On Unix ICC defines __GNUC__, but does not really
have all the features. Here ICC defines _MSC_VER, but does not behave
like cl.exe.

[I know it's a hard problem to be fully compatible, so I'm not blaming
the authors.]


What happens if you take the C99 path (starting in mpdecimal.c:48)?
msg249477 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-09-01 12:03
Steve: What do you mean by "global option"? The C99 standard says
that FENV_ACCESS (if set), is active until the end of the translation
unit (here: mpdecimal.c).
msg249488 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-09-01 16:27
fenv_access is not available when compiling with /fp:fast, which is apparently ICC's default.

The proposed workaround here changes that default to /fp:strict, which is a very different model, for all of CPython. I proposed using #pragma float_control to force /fp:strict locally and enable fenv_access, rather than changing the entire build to use /fp:strict (even though fenv_access is not enabled everywhere by default).
msg249490 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-09-01 16:34
Steve Dower added the comment:
> fenv_access is not available when compiling with /fp:fast, which is apparently ICC's default.
>
> The proposed workaround here changes that default to /fp:strict, which is a very different model, for all of CPython. I proposed using #pragma float_control to force /fp:strict locally and enable fenv_access, rather than changing the entire build to use /fp:strict (even though fenv_access is not enabled everywhere by default).

Note that I'm not suggesting changing the default for everything to
/fp:strict.  The goal of the project building Python with ICC on
Windows is to change nothing for an MSVC build.  The default floating
point model should not change without a good reason, and this is not
it :)

I'll experiment with your suggestions, Steve and Stefan, discuss it
with Intel, and get back to you here.
msg249493 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-09-01 16:41
If "#pragma float_control(precise, push)" is exactly the MSVC default
then I'm fine with putting it on top of FENV_ACCESS.

There's nothing speed sensitive going on here: In the 32-bit build
the x87 FPU is used for modular multiplication of integers and needs
the control word set to 80-bit prec + ROUND_CHOP.
msg249535 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-09-02 10:45
Also note that ICC <= 11.0 did not handle __GNUC__ inline asm
correctly, see the comment:

  Modules/_decimal/libmpdec/umodarith.h:391

I've disabled asm for Linux/ICC in setup.py.


I don't know how the situation is with MASM inline asm, but I'd
recommend to test at least a couple of exact multiplications
and divisions with operands of about 100000 digits (the asm is
only used for bignums).

There is already one bignum test in test_decimal that *should*
catch any oddities, but testing a bit more can't hurt.
msg249601 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-09-03 03:58
As far as I can tell, this patch fixes the issue and doesn't break anything.  Independent verification of that assertion would be lovely :)

For the record, I was able to reproduce the issue on one of the Windows Server 2012 R2 machines after installing ICC 16.0.  Weirdly, after that installation, the problem reproduces with both ICC 15.0 and 16.0.  I'm not sure why it compiled with ICC 15.0 before installing 16.0, although guessing from what the 16.0 installer was saying it replaced 15's VS 2015 integration with its own, so 15's may have had some bug in this area.
msg249624 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-09-03 09:34
Are you sure that you always tested the 32-bit build, also with ICC 15.0?
msg249668 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-09-03 16:02
I tested both 32 and 64 bit, with MSVC 14 (VS 2015) and ICC 15.0 (backed by VS 2015).

I can go ahead and commit this to a sandbox for buildbot testing if it would help.
msg249669 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-09-03 16:23
Hmm, I don't see a 32-bit ICC buildbot. This problem can only occur
on 32-bit with -DPPRO defined.


Please go ahead and commit the patch. The inline asm
miscompilation problem I mentioned earlier was solved in
12.0 (I think), so let's just assume it's gone (probably
it has never been an issue for MASM anyway).
msg249671 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-09-03 16:46
There's not a 32-bit ICC buildbot, though I could force one.

But since you say commit it, I'll commit it ;)
msg249672 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-09-03 16:52
We can always blame any fallout on ICC. ;)
msg249673 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-09-03 16:58
New changeset 863407e80370 by Zachary Ware in branch '3.5':
Issue #24974: Force fp-model precice in mpdecimal.c on Windows
https://hg.python.org/cpython/rev/863407e80370

New changeset 88c28d29afe4 by Zachary Ware in branch 'default':
Closes #24974: Merge with 3.5
https://hg.python.org/cpython/rev/88c28d29afe4
msg249674 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-09-03 16:59
Committed!  Thank you Steve for the suggestion and Stefan for the approval.
History
Date User Action Args
2022-04-11 14:58:20adminsetgithub: 69162
2015-09-03 16:59:30zach.waresetmessages: + msg249674
2015-09-03 16:58:39python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg249673

resolution: fixed
stage: patch review -> resolved
2015-09-03 16:52:14skrahsetmessages: + msg249672
2015-09-03 16:46:08zach.waresetmessages: + msg249671
2015-09-03 16:23:31skrahsetassignee: zach.ware
messages: + msg249669
2015-09-03 16:02:20zach.waresetmessages: + msg249668
2015-09-03 09:34:48skrahsetmessages: + msg249624
2015-09-03 03:58:49zach.waresetfiles: + _decimal_fp_precise_pragma.diff
priority: low -> normal
messages: + msg249601

keywords: + patch
stage: test needed -> patch review
2015-09-02 10:45:17skrahsetmessages: + msg249535
2015-09-01 16:41:08skrahsetmessages: + msg249493
2015-09-01 16:34:40zach.waresetmessages: + msg249490
2015-09-01 16:27:37steve.dowersetmessages: + msg249488
2015-09-01 12:50:58facundobatistasetnosy: - facundobatista
2015-09-01 12:03:21skrahsetmessages: + msg249477
2015-09-01 11:51:13skrahsetmessages: + msg249476
2015-09-01 05:21:52zach.waresetnosy: + christopher.hogan

title: ICC on Windows 8.1: _decimal fails to compile without /fp:strict -> ICC on Windows 8.1: _decimal fails to compile with default fp model
2015-08-31 22:12:18steve.dowersetmessages: + msg249434
2015-08-31 21:37:02zach.warecreate