msg249428 - (view) |
Author: Zachary Ware (zach.ware) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2015-09-03 16:52 |
We can always blame any fallout on ICC. ;)
|
msg249673 - (view) |
Author: Roundup Robot (python-dev) |
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) * |
Date: 2015-09-03 16:59 |
Committed! Thank you Steve for the suggestion and Stefan for the approval.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:20 | admin | set | github: 69162 |
2015-09-03 16:59:30 | zach.ware | set | messages:
+ msg249674 |
2015-09-03 16:58:39 | python-dev | set | status: open -> closed
nosy:
+ python-dev messages:
+ msg249673
resolution: fixed stage: patch review -> resolved |
2015-09-03 16:52:14 | skrah | set | messages:
+ msg249672 |
2015-09-03 16:46:08 | zach.ware | set | messages:
+ msg249671 |
2015-09-03 16:23:31 | skrah | set | assignee: zach.ware messages:
+ msg249669 |
2015-09-03 16:02:20 | zach.ware | set | messages:
+ msg249668 |
2015-09-03 09:34:48 | skrah | set | messages:
+ msg249624 |
2015-09-03 03:58:49 | zach.ware | set | files:
+ _decimal_fp_precise_pragma.diff priority: low -> normal messages:
+ msg249601
keywords:
+ patch stage: test needed -> patch review |
2015-09-02 10:45:17 | skrah | set | messages:
+ msg249535 |
2015-09-01 16:41:08 | skrah | set | messages:
+ msg249493 |
2015-09-01 16:34:40 | zach.ware | set | messages:
+ msg249490 |
2015-09-01 16:27:37 | steve.dower | set | messages:
+ msg249488 |
2015-09-01 12:50:58 | facundobatista | set | nosy:
- facundobatista
|
2015-09-01 12:03:21 | skrah | set | messages:
+ msg249477 |
2015-09-01 11:51:13 | skrah | set | messages:
+ msg249476 |
2015-09-01 05:21:52 | zach.ware | set | nosy:
+ 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:18 | steve.dower | set | messages:
+ msg249434 |
2015-08-31 21:37:02 | zach.ware | create | |