[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Zachary Ware]

Zachary Ware added the comment:

Committed!  Thank you Steve for the suggestion and Stefan for the approval.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Zachary Ware]

Zachary Ware added the comment:

There's not a 32-bit ICC buildbot, though I could force one.

But since you say commit it, I'll commit it ;)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Stefan Krah]

Stefan Krah added the comment:

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

--
assignee:  -> zach.ware

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Stefan Krah]

Stefan Krah added the comment:

We can always blame any fallout on ICC. ;)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Zachary Ware]

Zachary Ware added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Stefan Krah]

Stefan Krah added the comment:

Are you sure that you always tested the 32-bit build, also with ICC 15.0?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Zachary Ware]

Zachary Ware added the comment:

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.

--
keywords: +patch
priority: low -> normal
stage: test needed -> patch review
Added file: http://bugs.python.org/file40329/_decimal_fp_precise_pragma.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Stefan Krah]

Stefan Krah added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Stefan Krah]

Stefan Krah added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Stefan Krah]

Stefan Krah added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Facundo Batista]

Changes by Facundo Batista :


--
nosy:  -facundobatista

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Zachary Ware]

Zachary Ware added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Steve Dower]

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Stefan Krah]

Stefan Krah added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24974] ICC on Windows 8.1: _decimal fails to compile with default fp model

[Zachary Ware]

Changes by Zachary Ware :


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

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com