Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ENH, BUG: call emms after fmod on windows #17547

Closed
wants to merge 1 commit into from
Closed

ENH, BUG: call emms after fmod on windows #17547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions numpy/core/setup.py
View file
Open in desktop
Expand Up @@ -674,6 +674,12 @@ def get_mathlib_info(*args):
# Intel and Clang also don't seem happy with /GL
is_msvc = (platform.platform().startswith('Windows') and
platform.python_compiler().startswith('MS'))

# issue 16744: remove this code and the call to call_emms in nypmath
# when there is a fix from microsoft
if platform.platform().startswith('Windows') and sys.maxsize > 2**32:
npymath_sources.append(join('src', 'npymath', 'call_emms.obj'))

config.add_installed_library('npymath',
sources=npymath_sources + [get_mathlib_info],
install_dir='lib',
Expand Down
8 changes: 8 additions & 0 deletions numpy/core/src/npymath/call_emms.asm
View file
Open in desktop
@@ -0,0 +1,8 @@
.code
call_emms proc
emms
mov eax, 0
ret ; Return EAX
call_emms endp
end

Binary file added numpy/core/src/npymath/call_emms.obj
View file
Open in desktop
Binary file not shown.
43 changes: 39 additions & 4 deletions numpy/core/src/npymath/npy_math_internal.h.src
View file
Open in desktop
Expand Up @@ -371,6 +371,10 @@ NPY_INPLACE double npy_log2(double x)
* instead test for the macro, but I am lazy to do that for now.
*/

#ifdef _WIN64
int call_emms(void);
#endif

/**begin repeat
* #type = npy_longdouble, npy_float#
* #TYPE = NPY_LONGDOUBLE, FLOAT#
Expand Down Expand Up @@ -398,8 +402,8 @@ NPY_INPLACE @type@ npy_@kind@@c@(@type@ x)
/**end repeat1**/

/**begin repeat1
* #kind = atan2,hypot,pow,fmod,copysign#
* #KIND = ATAN2,HYPOT,POW,FMOD,COPYSIGN#
* #kind = atan2,hypot,pow,copysign#
* #KIND = ATAN2,HYPOT,POW,COPYSIGN#
*/
#ifdef @kind@@c@
#undef @kind@@c@
Expand All @@ -412,6 +416,21 @@ NPY_INPLACE @type@ npy_@kind@@c@(@type@ x, @type@ y)
#endif
/**end repeat1**/

#ifdef fmod@c@
#undef fmod@c@
#endif
#ifndef HAVE_FMOD@C@

NPY_INPLACE @type@ npy_fmod@c@(@type@ x, @type@ y)
{
@type@ ret = (@type@)npy_fmod((double)x, (double) y);
#ifdef _WIN64
call_emms();
#endif
return ret;
}
#endif

#ifdef modf@c@
#undef modf@c@
#endif
Expand Down Expand Up @@ -473,8 +492,8 @@ NPY_INPLACE @type@ npy_@kind@@c@(@type@ x)
/**end repeat1**/

/**begin repeat1
* #kind = atan2,hypot,pow,fmod,copysign#
* #KIND = ATAN2,HYPOT,POW,FMOD,COPYSIGN#
* #kind = atan2,hypot,pow,copysign#
* #KIND = ATAN2,HYPOT,POW,COPYSIGN#
*/
#ifdef HAVE_@KIND@@C@
NPY_INPLACE @type@ npy_@kind@@c@(@type@ x, @type@ y)
Expand All @@ -484,6 +503,21 @@ NPY_INPLACE @type@ npy_@kind@@c@(@type@ x, @type@ y)
#endif
/**end repeat1**/

#ifdef HAVE_FMOD@C@

NPY_INPLACE @type@ npy_fmod@c@(@type@ x, @type@ y)
{
@type@ ret = fmod@c@(x, y);
#ifdef _WIN64
call_emms();
#endif
return ret;
}
#endif




#ifdef HAVE_MODF@C@
NPY_INPLACE @type@ npy_modf@c@(@type@ x, @type@ *iptr)
{
Expand Down Expand Up @@ -757,3 +791,4 @@ npy_rshift@u@@c@(npy_@u@@type@ a, npy_@u@@type@ b)
}
/**end repeat1**/
/**end repeat**/

13 changes: 10 additions & 3 deletions numpy/distutils/command/build_clib.py
View file
Open in desktop
Expand Up @@ -289,16 +289,22 @@ def build_a_library(self, build_info, lib_name, libraries):
# problem, msvc uses its own convention :(
c_sources += cxx_sources
cxx_sources = []
obj_ext = '.obj'
else:
obj_ext = '.o'

objects = []
# c_sources collects all the default sources, including pre-compiled
# object files
objects = [c_sources.pop(c_sources.index(c)) for c in c_sources[:]
if c.endswith(obj_ext)]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuk. sorry for this, if there is a better way please suggest it. The c_sources is from

c_sources, cxx_sources, f_sources, fmodule_sources \
            = filter_sources(sources)

above, and is a bit misnamed since it collects anything that is not cxx_sources, f_source or fmodule_sources. Also see line 264 where dispatch_sources does something similar

Copy link
Member

@pv pv Oct 13, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be done instead by passing in a .lib file with libraries + library_dirs from setup.py, so distutils hacking is not needed?

Copy link
Member

@pv pv Oct 13, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can the .lib file be compiled from sources with MASM in the setup.py file, so that the binary blob doesn't need to be shipped with Numpy? I guess the only info you'd need from distutils for that is the location of the msvc executables --- the MASM command line can be hardcoded as it's anyway platform specific.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, thanks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will require more hacking than the current solution. setup.py is not really the place to be running compilation, that is the job of config.add_library and friends. Is shipping a 1k binary object file that is easily dissasembled a deal breaker?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an example around of using MASM from python in a setup.py ? Maybe I am not seeing something obvious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, got it. There is an extra_objects arg to Extensions

Copy link
Member

@pv pv Oct 14, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping it would be simple to do with running via subprocess.run the commands that you would do manually, i.e., minimize interfacing with distutils at all. And passing the generated file to the Extension. But maybe it's not so simple.

I'd guess shipping binary blobs is not a dealbreaker, given that hopefully this hack can be dropped at some point in future, after the CRT bug is fixed.

Copy link
Member

@carlkl carlkl Oct 14, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe pycca can be used for that assembler snippet, but it would be another dependency.

if c_sources:
log.info("compiling C sources")
objects = compiler.compile(c_sources,
objects.extend(compiler.compile(c_sources,
output_dir=self.build_temp,
macros=macros,
include_dirs=include_dirs,
debug=self.debug,
extra_postargs=extra_args_baseopt)
extra_postargs=extra_args_baseopt))
objects.extend(dispatch_objects)

if cxx_sources:
Expand Down Expand Up @@ -392,3 +398,4 @@ def build_a_library(self, build_info, lib_name, libraries):
clib_libraries.extend(binfo.get('libraries', []))
if clib_libraries:
build_info['libraries'] = clib_libraries