msvc9 32-bit builds are unusable #6428
Comments
|
@juliantaylor Thoughts? |
|
Note that one quick solution is to revert the patch. @juliantaylor What compilers were affected by the original code? |
|
@cgohlke Sounds almost like a compiler bug. @juliantaylor I suppose we could put an |
|
can we get a disassembly of these functions? |
|
btw I do have a windows32 (virtual) box, next time you can ask me to test the binaries you produced before releasing them. |
|
oh wait out binaries are built with mingw and work (I assume), so this is not actually such a big problem |
|
@juliantaylor that's why I haven't taken down 1.10.0 on Sourceforge yet. But that doesn't mean it can't affect someone, somewhere. We could also solve the problem by dropping Python 2.6 support... |
|
Looks like Python 2.6 was officially retired with 2.6.9 released in Oct 2013. |
|
yes we should try to fix it, though I don't really know how, unless we understand the problem it would have to be a versioned ifdef for msvc9. The old code invokes undefined behaviour so we do not want to just revert the change. I think dropping py2 (and < py3.5) support on windows is not unreasonable from a technical standpoint. unfortunately the py2.7 downloads still exceed py3 by a large margin. |
|
Oh, right. Looks like python 2.7 also uses msvc9. Darn. |
|
All Pythons up to 3.2, in fact. I'd like to drop 3.2 at some point but I expect it will be a long time before we can drop 2.7. |
|
There is this http://www.microsoft.com/en-us/download/details.aspx?id=44266, which is a snapshot of 9.0.0.30729. @cgohlke What version of msvc9 are you using? |
|
I am using Visual Studio 2008 Pro with SP1 installed, that is version 15.00.30729.01. |
|
Here are the assembly output with and without #6252: _TEXT ENDS
PUBLIC __real@00000000
; COMDAT __real@00000000
CONST SEGMENT
__real@00000000 DD 000000000r ; 0
; Function compile flags: /Ogtpy
CONST ENDS
_TEXT SEGMENT
_tmp$ = -4 ; size = 4
_a$ = 8 ; size = 4
_b$ = 12 ; size = 4
_sse2_ordered_cmp_equal_FLOAT PROC
; 541 : {
push ebp
mov ebp, esp
and esp, -16 ; fffffff0H
sub esp, 16 ; 00000010H
; 542 : @type@ tmp;
; 543 : @vtype@ v = @vpre@_@VOP@_@vsufs@(@vpre@_load_@vsufs@(&a),
; 544 : @vpre@_load_@vsufs@(&b));
; 545 : @vpre@_store_@vsufs@(&tmp, v);
; 546 : return !(tmp == 0.);
fldz
xorps xmm0, xmm0
movss xmm0, DWORD PTR _a$[ebp]
xorps xmm1, xmm1
movss xmm1, DWORD PTR _b$[ebp]
cmpeqss xmm0, xmm1
movss DWORD PTR _tmp$[esp+16], xmm0
fcomp DWORD PTR _tmp$[esp+16]
fnstsw ax
test ah, 68 ; 00000044H
jnp SHORT $LN3@sse2_order
mov eax, 1
; 547 : }
mov esp, ebp
pop ebp
ret 0
$LN3@sse2_order:
; 542 : @type@ tmp;
; 543 : @vtype@ v = @vpre@_@VOP@_@vsufs@(@vpre@_load_@vsufs@(&a),
; 544 : @vpre@_load_@vsufs@(&b));
; 545 : @vpre@_store_@vsufs@(&tmp, v);
; 546 : return !(tmp == 0.);
xor eax, eax
; 547 : }
mov esp, ebp
pop ebp
ret 0
_sse2_ordered_cmp_equal_FLOAT ENDP; Function compile flags: /Ogtpy
_tmp$ = -4 ; size = 4
_a$ = 8 ; size = 4
_b$ = 12 ; size = 4
_sse2_ordered_cmp_equal_FLOAT PROC
; 541 : {
push ebp
mov ebp, esp
and esp, -16 ; fffffff0H
sub esp, 16 ; 00000010H
; 542 : @type@ tmp;
; 543 : @vtype@ v = @vpre@_@VOP@_@vsufs@(@vpre@_load_@vsufs@(&a),
; 544 : @vpre@_load_@vsufs@(&b));
xorps xmm0, xmm0
movss xmm0, DWORD PTR _a$[ebp]
xorps xmm1, xmm1
movss xmm1, DWORD PTR _b$[ebp]
cmpeqss xmm0, xmm1
; 545 : @vpre@_store_@vsufs@(&tmp, v);
movss DWORD PTR _tmp$[esp+16], xmm0
; 546 : return sizeof(@type@) == 4 ?
; 547 : (*(npy_uint32 *)&tmp) & 1 : (*(npy_uint64 *)&tmp) & 1;
mov eax, DWORD PTR _tmp$[esp+16]
and eax, 1
; 548 : }
mov esp, ebp
pop ebp
ret 0
_sse2_ordered_cmp_equal_FLOAT ENDP |
|
NVM, I missed the jump. |
|
However, there is an extra compare in the new version. Although it it uses test and apparently looks specifically at the bits in tmp. Could be that doesn't handle NaNs correctly? |
|
I'm going to guess the |
|
I wonder what is different with 64 bits? |
|
nan shouldn't really matter, in fact if the comparison is true tmp is always nan but nan is not equal 0 the difference for 64 bit is probably that it doesn't use the x87 fpu which will lead to different code generation |
|
Easy to check by not inlining it. |
|
It seem compiling with |
|
ok got the 2.7 sdk to compile numpy, we should probably see if we can make this easier, setting magical environent variables to build numpy is not very ideal (SET DISTUTILS_USE_SDK=1 andSET MSSdk=1) just using isnan(tmp) seems to work but I still get lots of comparison failed deprecation warnings also when I turn of sse, weird. |
|
SSE2 has been available since 2001. |
msvc2008 32 bit seems to miscompile it otherwise. closes numpygh-6428
|
the warnings seem normal with this compiler, should probably be looked at, for now filed a PR with a reasonable fix |
|
Firefox crash data from 20 September gives 0.5 percent of users without
SSE2:
https://gist.github.com/matthew-brett/9cb5274f7451a3eb8fc0
https://crash-analysis.mozilla.com/crash_analysis/20150920/20150920-pub-crashdata.csv.gz
|
|
Looks like two solutions now, thanks guys. The SSE2 solution might be risky. I'll wait until tomorrow for things to sort out, then see to a 1.10.1 release. |
|
Yeah, I think requiring SSE2 is probably a good plan in general, but
|
|
With #6438 the following test errors remain (#6440 fixes these too): |
|
Hmm, that is strange set of errors to be fixed by SSE2. |
|
Maybe inf related? |
|
I'm thinking the SSE2 solution for msvc9 is the way to go. My reasoning is
|
|
Two things to consider for the SSE2 solution:
|
|
@cgohlke If I understand correctly, msvc > 9 works, and gcc seems to have no problems with the same code, so I am going with compiler bug. Your second point is a concern, and perhaps the only way to settle that is to give it a try. @rgommers Maybe we can get Ralf to take a moment out of reviewing SciPy PRs to offer an opinion. |
|
I wonder how many people are even using msvc9 to build numpy... I know that
|
|
If enabling SSE2 on Windows works for numpy itself, then I don't really see a problem with enabling it for other packages using I'd still prefer a nice warning for those few users that are still without SSE2, but each time we discuss this 6-12 months have passed so my preference becomes less and less relevant.... maybe the time has come to just pull the trigger. |
|
note that numpy used sse2 when built with msvc since 1.8, just not for every part of the code |
|
Another interesting note on SSE2: apparently Python 3.5 on Windows requires On Sat, Oct 10, 2015 at 11:45 AM, Julian Taylor notifications@github.com
Nathaniel J. Smith -- http://vorpus.org |
|
@njsmith we use msvc 9 to build every package on 2.7. |
|
@cournape: are you ok with enabling SSE2 in 2.7 builds?
|
|
Re-opening because I'm not sure #6438 is still intended as the final fix for this so someone who understands what's going on better should probably close it by hand instead of trusting the auto close mechanism. |
msvc2008 32 bit seems to miscompile it otherwise. closes numpygh-6428
|
I will take a look tomorrow to see if we can get some idea of the proportion of our customers w/ a CPU that is not SSE2 enabled. |
|
@cournape: note also #6428 (comment) |
|
Also my comment up this thread : #6428 (comment) - Firefox crash reports : SSE2 == 99.5% Steam hardware survey : SSE2 == 99.99% |
|
unless you are not shipping numpy 1.8 yet its irrelevant, its already required since 1.8 with msvc builds |
|
Numpy-1.10.1 is passing all tests. Closing. |
Thanks to @cgohlke and @JulianTay for getting this fixed.
msvc2008 32 bit seems to miscompile it otherwise. closes numpygh-6428
|
The |
|
@charris could you please comment on comment-146975795 at http://stackoverflow.com/q/38270589/648265 (or wherever)? Looks like what is says is not as true and common knowledge as you make it sound to be. |
numpy-1.10.0 compiled with Visual Studio 2008 for 32-bit Python fails many tests (only part of the output is shown):
Reverting #6252 fixes this problem.
Builds using msvc9 64-bit, msvc10, msvc14, and icl do not show this problem.
I did not notice this before because I was testing the RCs with the Intel, not msvc, compilers.
The text was updated successfully, but these errors were encountered: