-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Missing scalar fp exceptions #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I'm a bit dubious about this fix. Did you check the compiler output to make sure it was messing up? Is it inlining the function calls? It looks to me like the compiler should be able to tell that the variable can change since it is passed by reference, so something more subtle may be going on. There is a similar example in a very old ticket where moving some code changed the error behaviour and I don't really know why. My thoughts were some sort of timing/thread problems calling back into python to get the err state. |
I didn't check the compiler output, but in this case it's macros. I did, however, try replacing them with functions, but that didn't work, presumably because inlining gave the equivalent effect. The half stuff I wrote for this part uses function calls to a different module, and it works correctly. My reasoning about the reordering was that arg1, arg2, and out are all local variables, and not passed as parameters to PyUFunc_getfperr. Thus, the values of those variables cannot affect getfperr, and getfperr cannot affect their values, and switching the operation to after getfperr (probably after the if statement) would be a reasonable optimization. The correct compiler fix would be for the compiler to have a generate a decoration which indicates whether a function uses or affects the floating point state, then use that information as a reordering constraint, but I have no idea whether gcc tries to do this. The error-checking code doesn't call back into Python for the err state; It calls a light wrapper around the UFUNC_CHECK_STATUS macro which calls one the various appropriate system functions: https://github.com/numpy/numpy/blob/master/numpy/core/include/numpy/ufuncobject.h#L253 I'll take a look at the assembly. |
Here's the assembly without my patch. Notice that the @Oper@ part is just doing some data copies.
At the end, there's this, where it's doing the divide just before the return.
|
Here are the same two parts with the patch:
and (I'm guessing the assign was optimized upwards)
|
OK, I'm convinced. But then it looks like this problem might very well occur in other spots. I wonder if we can think of a better solution than going around marking variables as volatile. |
To fix this type of bug in general, we effectively want PyUFunc_*fperr() to operate as compiler memory barriers, described here: I'm guessing putting the barrier as described inside the PyUFunc_*fperr() wouldn't propagate up into the callers, so maybe we could rename those functions, and introduce macros such as this:
|
What a mess. The code has side effects and I don't know how to make that clear to the compiler. I suspect you are right about not being able to put the barrier inside the PyUFunc_*fperr() functions unless we want to provide an error return. That might be something to consider. |
Maybe MSVC doesn't have the same bug, but their compiler memory barrier didn't propagate up the call tree until Visual C++ 2010. See the note in the Remarks section. http://msdn.microsoft.com/en-us/library/f20w0x5e(v=VS.100).aspx |
Out of curiousity, what compiler/optimization flags are you using? Using volatile here bothers me because I'm not sure the semantics of volatile are that clear.Especially when applied to a variable that isn't volatile. Trying to game the compiler might not be the best way to go here. Thinking about it a bit more, checking the floating point flags has got to be a common operation and the compiler has got to deal. But it doesn't know we are checking the flags because that operation is buried in a function in another module. What happens if you use the macro directly? |
I'm running Fedora 13 x86_64, gcc (GCC) 4.4.4 20100630 (Red Hat 4.4.4-10). The compiler options are all the NumPy default, here's what it prints out during the build: C compiler: gcc -pthread -DNDEBUG -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -D_GNU_SOURCE -fPIC -fwrapv -g -fPIC |
Curiously, using the purported gcc memory barrier failed to work. The divss still gets shuffled to the end. |
I have the same gcc version and also see the error. Oddly enough, using the macro directly didn't work either, and I would regard that as a compiler bug since it makes it impossible to check if a particular floating point operation has set an exception flag. Using fetestexcept (defined in fenv.h) should separate code into before and after, although I don't think C provides a good method for that. The order in which volatile variables are accessed needs to be kept intact and I trying think of a way of using that. |
And there is actually a bug report against gcc: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=29186. Which was originally against 4.1 in 2006 and it still hasn't been dealt with. Hmmm... there were various workarounds proposed but non of them strike me as dependable. It is possible to turn off some optimizations with -fno-tree-sink which apparently makes a difference, but strikes me as a bit of a hack. |
And it doesn't look like this issue will be fixed anytime soon. GCC still hasn't implemented #pragma STDC FENV_ACCESS ON. Stackoverflow suggest D and Fortran-2003 as languages that actually allow floating point error tracking. If @name@ctype@Oper@ were actual functions we could have them set a dummy variable that gets passed to PyUFunc_getfperr, but that might could get bypassed with inlining. This all makes me appreciate a fine rant I read yesterday about how C, Java, and Fortran don't provide any way to reliably track floating point (and integer) errors and lamenting that the situation is far worse than it was 30 years ago. |
One thought I had was to have PyUFunc_getfperr take a void* parameter, and pass &out to it. This way, the compiler would need to assume that the function uses whatever value is in out for some purpose, and would calculate its value before calling the function (same as what adding a printf in between did). It's kind of annoying to design the API based on compiler bugs, though, I agree it should just work. In my opinion, I should be able to reliably do things Kahan suggests like set rounding modes to +/- inf to help test robustness, etc, but in this case something simple like just detecting the FP state is unreliable... |
Yeah, I've had similar thoughts. Another possibility is have the computation set a done flag and put the test inside an if statement. The setting of done would have to hidden from the compiler, however. This is like using out as a flag, but I think using out directly in that capacity a bit mysterious. What would be nice is some sort of macro that implements a run then check scenario. |
This sort of thing works, but looks ugly. static attribute ((noinline)) int .... retstatus = checkexpression(&out); But as you say, it's ugly. |
I was thinking of going a bit further than that, and actually changing the signature of PyUFunc_getfperr(). This way, any code, either in NumPy or in modules that link to NumPy, needs to be changed, with whoever changing it hopefully reading some documentation about the issue and getting the opportunity to take appropriate action. Since it's for 2.0, this kind of API change might be acceptable? I tried changing PyUFunc_getfperror(void) to PyUFunc_getfperror(void *op_target_ptr), and this fixed it as well. Also, any usage of the UFUNC_CHECK_STATUS macro also potentially has this bug. Perhaps it should be removed from the public-facing API entirely? |
I agree that we have to do something here as a workaround and something like that, ugly as it is, seems like the most reliable and portable way to enforce ordering. But this looks like another one of those things that needs to be discussed on the list, if only to provide something for searches. You have a knack for finding these deep issues ;) |
I've looked through all the usages of PyUFunc_getfperr and UFUNC_CHECK_STATUS, and it looks like this was the only place it caused a bug. The @type@power function was the only other scalar op candidate, but it works correctly. All the UFUNC* macro usages are ok, I believe, because they're going through function pointers in the ufunc machinery. I tweaked the commit to put in a more descriptive comment, and the unit test is more comprehensive now. |
Sorry for the multiple little tweaks - I added the power function into the unit test to be a bit more thorough. |
I cleaned up the white space and broke long lines. You can check it at |
Looks good. Three small tweaks: The rewording to The docstring tweak mentions Remove the extra '' here: |
ENH: Add integrated legacy generator
BUG: Cast high to Python int to avoid overflow
# This is the 1st commit message: Add Windows config to GHA # This is the commit message numpy#2: update script [wheel build] # This is the commit message numpy#3: typo [wheel build] # This is the commit message numpy#4: fix typo? [wheel build] # This is the commit message numpy#5: fix linux builds? [wheel build] # This is the commit message numpy#6: typo [wheel build] # This is the commit message numpy#7: add license and pin to windows 2016 # This is the commit message numpy#8: skip tests [wheel build] # This is the commit message numpy#9: pin to windows 2019 instead [wheel build] # This is the commit message numpy#10: try to find out the error on windows [wheel build] # This is the commit message numpy#11: maybe fix? [wheel build] # This is the commit message numpy#12: maybe fix? [wheel build] # This is the commit message numpy#13: fix? [wheel build] # This is the commit message numpy#14: cleanup [wheel build]
commit 30228578ac65ec6b90961700a5783c23d8e1b879 Author: Thomas Li <47963215+lithomas1@users.noreply.github.com> Date: Wed Nov 17 16:21:27 2021 -0800 Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Update LICENSE_win32.txt Update LICENSE_win32.txt Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Update LICENSE_win32.txt Update LICENSE_win32.txt Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Update LICENSE_win32.txt Update LICENSE_win32.txt Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Update LICENSE_win32.txt Update LICENSE_win32.txt commit 4bd12df Author: Thomas Li <47963215+lithomas1@users.noreply.github.com> Date: Mon Nov 15 17:28:47 2021 -0800 # This is a combination of 14 commits. # This is the 1st commit message: Add Windows config to GHA # This is the commit message numpy#2: update script [wheel build] # This is the commit message numpy#3: typo [wheel build] # This is the commit message numpy#4: fix typo? [wheel build] # This is the commit message numpy#5: fix linux builds? [wheel build] # This is the commit message numpy#6: typo [wheel build] # This is the commit message numpy#7: add license and pin to windows 2016 # This is the commit message numpy#8: skip tests [wheel build] # This is the commit message numpy#9: pin to windows 2019 instead [wheel build] # This is the commit message numpy#10: try to find out the error on windows [wheel build] # This is the commit message numpy#11: maybe fix? [wheel build] # This is the commit message numpy#12: maybe fix? [wheel build] # This is the commit message numpy#13: fix? [wheel build] # This is the commit message numpy#14: cleanup [wheel build] commit 444a721 Merge: 376ad69 22448b4 Author: Charles Harris <charlesr.harris@gmail.com> Date: Mon Nov 15 17:47:23 2021 -0700 Merge pull request numpy#20274 from h-vetinari/fix_15179 TST: Some fixes & refactoring around glibc-dependent skips in test_umath.py commit 376ad69 Merge: b75fe57 546c47a Author: Charles Harris <charlesr.harris@gmail.com> Date: Mon Nov 15 17:31:41 2021 -0700 Merge pull request numpy#20327 from seberg/rename-interpolation BUG,DEP: Fixup quantile/percentile and rename interpolation->method commit 546c47a Author: Sebastian Berg <sebastian@sipsolutions.net> Date: Mon Nov 15 16:13:50 2021 -0600 DOC: Fixups for interpolation rename comments from review Co-authored-by: Charles Harris <charlesr.harris@gmail.com> commit b75fe57 Merge: 7310c09 cbc25d2 Author: Warren Weckesser <warren.weckesser@gmail.com> Date: Mon Nov 15 17:27:08 2021 -0500 Merge pull request numpy#20369 from bilderbuchi/missing_diagnostics_newlines MAINT: Fix newlines in diagnostics output of numpy.f2py. commit cbc25d2 Author: Christoph Buchner <bilderbuchi@phononoia.at> Date: Sun Nov 14 08:36:03 2021 +0100 MAINT: Fix newlines in diagnostics output of numpy.f2py. Linebreaks were not consistently added to errmess/outmess arguments, which led to very long lines and wrong concatenation with compiler messages in f2py console output. commit be15716 Author: Sebastian Berg <sebastian@sipsolutions.net> Date: Fri Nov 12 12:10:20 2021 -0600 DOC: Add release not for quantile `interpolation` rename Also updates the old release note to include the info (and generally tweaks it a bit) commit 7d8a8e7 Author: Sebastian Berg <sebastian@sipsolutions.net> Date: Fri Nov 12 11:57:22 2021 -0600 DOC: Update percentile/quantile docs Mainly fixes the method list slightly, tones down the warning a bit and fixes the link to the paper (I did not realize that the link failed to work due only because the reference was missing from nanquantile/nanpercentile). commit 5bd71fb Author: Sebastian Berg <sebastian@sipsolutions.net> Date: Tue Nov 9 09:48:59 2021 -0600 DOC: Add ticks to quantile interpolation/method error Co-authored-by: abel <aoun@cerfacs.fr> commit 0d5fb81 Author: Sebastian Berg <sebastian@sipsolutions.net> Date: Mon Nov 8 20:39:50 2021 -0600 DOC: Remove reference to paper from quantile `method` kwarg Apparently, sphinx does not resolve references to footnotes from parameter descriptions. commit 8437663 Author: Sebastian Berg <sebastian@sipsolutions.net> Date: Mon Nov 8 18:25:37 2021 -0600 MAINT: Rename interpolation to method in percentile stubs commit 8cfb6b5 Author: Sebastian Berg <sebastian@sipsolutions.net> Date: Mon Nov 8 16:47:27 2021 -0600 TST: Add deprecation testcase for quantile interpolation rename commit a5ac5a5 Author: Sebastian Berg <sebastian@sipsolutions.net> Date: Mon Nov 8 16:41:24 2021 -0600 DOC: Fixup the percentile methods plot commit 85f3dda Author: Sebastian Berg <sebastian@sipsolutions.net> Date: Mon Nov 8 16:37:41 2021 -0600 BUG: quantile discrete methods ended up using -1 as index sometimes Also, the closest-observation did not correctly support multiple quantiles calculated at the same time (broadcasting error). commit 3993408 Author: Sebastian Berg <sebastian@sipsolutions.net> Date: Mon Nov 8 15:38:30 2021 -0600 API,DEP: Rename percentile/quantile `interpolation=` to `method=` commit 22448b4 Author: H. Vetinari <h.vetinari@gmx.com> Date: Tue Nov 2 16:51:59 2021 +1100 TST: parametrize glibc-version check in test_umath.py commit 12923c2 Author: H. Vetinari <h.vetinari@gmx.com> Date: Tue Nov 2 14:21:01 2021 +1100 TST: skip coverage of large elements in sincos_float32 for ancient glibc Fixes numpy#15179 commit 56268d5 Author: H. Vetinari <h.vetinari@gmx.com> Date: Tue Nov 2 14:19:24 2021 +1100 TST: use existence of glibc-version to clean up a test commit 01443e8 Author: H. Vetinari <h.vetinari@gmx.com> Date: Tue Nov 2 14:18:52 2021 +1100 TST: turn glibc_older_than_2.17 into boolean instead of xfail this anticipates reuse of this boolean within the test module
commit 9c833bed5879d77e625556260690c349de18b433 Author: Thomas Li <47963215+lithomas1@users.noreply.github.com> Date: Wed Nov 17 16:21:27 2021 -0800 Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Update LICENSE_win32.txt Update LICENSE_win32.txt Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Update LICENSE_win32.txt Update LICENSE_win32.txt Update cibw_test_command.sh commit 4bd12df Author: Thomas Li <47963215+lithomas1@users.noreply.github.com> Date: Mon Nov 15 17:28:47 2021 -0800 # This is a combination of 14 commits. # This is the 1st commit message: Add Windows config to GHA # This is the commit message numpy#2: update script [wheel build] # This is the commit message numpy#3: typo [wheel build] # This is the commit message numpy#4: fix typo? [wheel build] # This is the commit message numpy#5: fix linux builds? [wheel build] # This is the commit message numpy#6: typo [wheel build] # This is the commit message numpy#7: add license and pin to windows 2016 # This is the commit message numpy#8: skip tests [wheel build] # This is the commit message numpy#9: pin to windows 2019 instead [wheel build] # This is the commit message numpy#10: try to find out the error on windows [wheel build] # This is the commit message numpy#11: maybe fix? [wheel build] # This is the commit message numpy#12: maybe fix? [wheel build] # This is the commit message numpy#13: fix? [wheel build] # This is the commit message numpy#14: cleanup [wheel build]
commit 9c833bed5879d77e625556260690c349de18b433 Author: Thomas Li <47963215+lithomas1@users.noreply.github.com> Date: Wed Nov 17 16:21:27 2021 -0800 Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Update LICENSE_win32.txt Update LICENSE_win32.txt Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Update LICENSE_win32.txt Update LICENSE_win32.txt Update cibw_test_command.sh commit 4bd12df Author: Thomas Li <47963215+lithomas1@users.noreply.github.com> Date: Mon Nov 15 17:28:47 2021 -0800 # This is a combination of 14 commits. # This is the 1st commit message: Add Windows config to GHA # This is the commit message numpy#2: update script [wheel build] # This is the commit message numpy#3: typo [wheel build] # This is the commit message numpy#4: fix typo? [wheel build] # This is the commit message numpy#5: fix linux builds? [wheel build] # This is the commit message numpy#6: typo [wheel build] # This is the commit message numpy#7: add license and pin to windows 2016 # This is the commit message numpy#8: skip tests [wheel build] # This is the commit message numpy#9: pin to windows 2019 instead [wheel build] # This is the commit message numpy#10: try to find out the error on windows [wheel build] # This is the commit message numpy#11: maybe fix? [wheel build] # This is the commit message numpy#12: maybe fix? [wheel build] # This is the commit message numpy#13: fix? [wheel build] # This is the commit message numpy#14: cleanup [wheel build]
Merge in ~STEPAN.SINDELAR_ORACLE.COM/numpy-hpy from fa/nditer2 to labs-hpy-port * commit 'b06d48d5f27887fe147801235142468758ab648b': (46 commits) Revert "Temporarily add missing scalarapi.h" Do appropriate include for alloca Introduce HNPY_cast_info, use HPyField in PyArrayMethodObject, migrate function signature 'resolve_descriptors_function' to HPy. Convert PyArrayMethod_Type to legacy HPy type Migrate define_cast_for_descrs, init_cast_info, PyArray_GetCastingImpl Use HPyArray_AdaptDescriptorToArray in hnpyiter_prepare_one_operand Do minimal migration of find_descriptor_from_array Migrate HPyArray_ExtractDTypeAndDescriptor to use HPyGlobal Migrate nditer_constr to use HPyGlobal Include hpy_utils.h in common.h Add HPyGlobal_Is to hpy_utils.c Migrate PyArray_ExtractDTypeAndDescriptor Introduce HPyGlobal HPyArrayDTypeMeta_Type Migrate PyArray_FailUnlessWriteable Introduce legacy accessors NIT_PY_OPERANDS and NIT_PY_DTYPES Finish minimal migration of HNpyIter_AdvancedNew Migrate PyArray_GetMaskedDTypeTransferFunction and PyArray_GetDTypeTransferFunction Temporarily add missing scalarapi.h Use HPyArray_NewFromDescr Add HPyArray_NewFromDescr to ctors.h ...
feat: Add vmov_n_s8
http://projects.scipy.org/numpy/ticket/1671
This patch fixes the errors on my machine, and adds tests for them. It appears to have been an over-eager optimizer, fixed by making a variable volatile, which gcc uses as a signal to prevent reordering.