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.

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

ENH: Add changes that allow NumPy to compile with clang-cl #20866

Merged
merged 1 commit into from Jan 23, 2022

Conversation

bashtage
Copy link
Contributor

Port over changes needed to compile NumPy using clang-cl
from #13816. Desirable to retain these even though distutils is
effectively deprecated and so not worth the effort to alter.

Port over changes needed to compile NumPy using clang-cl
from numpy#13816. Desirable to retain these even though distutils is
effectively deprecated and so not worth the effort to alter.
@rgommers
Copy link
Member

Cc @serge-sans-paille FYI - the header changes may be relevant for Pythran perhaps, since it requires clang-cl on Windows.

@rgommers rgommers added the 36 - Build Build related PR label Jan 20, 2022
@@ -41,7 +41,7 @@
* @param dtype2 Second DType class.
* @return The common DType or NULL with an error set
*/
NPY_NO_EXPORT NPY_INLINE PyArray_DTypeMeta *
NPY_NO_EXPORT PyArray_DTypeMeta *
Copy link
Member

Choose a reason for hiding this comment

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

What is the problem with NPY_INLINE here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it is no export and inline, clang-cl only lets it be used in the same translation unit. It is later linked across object files which makes clang-cl error.

@charris charris merged commit 72a27ad into numpy:main Jan 23, 2022
@charris
Copy link
Member

charris commented Jan 23, 2022

Thanks @bashtage. Should this be backported?

@bashtage
Copy link
Contributor Author

I wouldn't bother. NumPy can't actually be compiled using clang-cl since it requires support in NumPy distutils, which is not deprecated. I have mostly given up on #13816 which contained the distutils choice. I'm hoping that when a new build system is in place that it will automatically setting alternative compilers on Windows (e.g., icc or clang-cl), so that these patches will be useful at that point.

@charris
Copy link
Member

charris commented Jan 24, 2022

Since it is no export and inline, clang-cl only lets it be used in the same translation unit.

That actually sounds like a clang-cl problem, the function isn't marked static. OTOH, it might be one of those murky areas.

@mattip
Copy link
Member

mattip commented Jul 7, 2023

@bashtage do you know if the meson build, which is the default on main, works now with clang-cl cleanly? It should be chosen "automagically" if clang can be found on the path when doing pip install . or spin build or meson setup build. On the CI builds we force MSVC by using --vsenv.

You may need to install pkg-config and set PKG_CONFIG_PATH to get OpenBLAS as we do here

choco install -y --checksum 6004DF17818F5A6DBF19CB335CC92702 pkgconfiglite

and here
- powershell: |
$ErrorActionPreference = "Stop"
mkdir C:/opt/openblas/openblas_dll
mkdir C:/opt/32/lib/pkgconfig
mkdir C:/opt/64/lib/pkgconfig
# TBD: support 32 bit testing
$target=$(python -c "import tools.openblas_support as obs; plat=obs.get_plat(); ilp64=obs.get_ilp64(); target=f'openblas_{plat}.zip'; obs.download_openblas(target, plat, ilp64);print(target)")
unzip -o -d c:/opt/ $target
echo "##vso[task.setvariable variable=PKG_CONFIG_PATH]c:/opt/64/lib/pkgconfig"

@bashtage
Copy link
Contributor Author

I haven't tried usign meson. I'll give it a go this week and let you know if ti works without further modification. It has been a while since I've tried and it is possible that there has been some drift int he code base that might need change.

@bashtage
Copy link
Contributor Author

@mattip

I made some progress but not compiling yet. Not very familiar with meson, so some of this might be wrong.

  1. Apply this small patch
--- a/numpy/core/src/multiarray/_multiarray_tests.c.src
+++ b/numpy/core/src/multiarray/_multiarray_tests.c.src
@@ -1966,7 +1966,7 @@ get_fpu_mode(PyObject *NPY_UNUSED(self), PyObject *args)
         return NULL;
     }

-#if defined(_MSC_VER)
+#if defined(_MSC_VER) && !defined(__clang__)
     {
         unsigned int result = 0;
         result = _controlfp(0, 0);
  1. Create a custom in file clang-cl-build.ini
[binaries]
c = 'clang-cl'
cpp = 'clang-cl'
ar = 'llvm-lib'
  1. Run
 meson setup build --native-file clang-cl.ini --prefix=$PWD\build-install -Ddebug=false --optimization 2
  1. Run python -m pip wheel . -vv

The last step errors with

[238/261] Linking target numpy/core/_multiarray_umath.cp311-win_amd64.pyd
  FAILED: numpy/core/_multiarray_umath.cp311-win_amd64.pyd
  "clang++" @numpy/core/_multiarray_umath.cp311-win_amd64.pyd.rsp
  clang++: error: no such file or directory: 'numpycorelibnpymath.a'

  clang++: error: no such file or directory: 'C:Anacondaenvsnumpy-devlibspython311.lib'

Looks like some of the wrong types of slashes are bring used and are somehow being eaten. Any idea where these commands are?

@rgommers
Copy link
Member

An alternative to an ini file is to set environment variables like CC and CXX: https://mesonbuild.com/Reference-tables.html#compiler-and-linker-selection-variables. Not sure about ar, I'd hope that if clang-cl needs that it's automatically selected when CC and CXX are set, but not sure.

No idea what's going on with the file path there, or what a .pyd.rsp file is.

@mattip
Copy link
Member

mattip commented Jul 11, 2023

Since windows has a line-length limit, msvc has the ability to read more commands from a response file instead. So ignore that and pretend the commands all came from the command line.

I don't see any meson open issues around path slash handling. Maybe connected to np.get_include() and sysconfig returning the wrong slashes?

@bashtage
Copy link
Contributor Author

This mostly works

python -m pip wheel . --config-settings=setup-args="--native-file=$PWD\clang-cl-build.ini" -vv

where the clang-cl-build.ini is

[binaries]
c = 'clang-cl'
cpp = 'clang-cl'
ar = 'llvm-lib'
c_ld = 'lld-link'
cpp_ld = 'lld-link'

This can be created using the command

 "[binaries]","c = 'clang-cl'","cpp = 'clang-cl'","ar = 'llvm-lib'","c_ld = 'lld-link'","cpp_ld = 'lld-link'"  | Out-File $PWD/clang-cl-build.ini

Two tests fail with

FAILED core/tests/test_einsum.py::TestEinsum::test_einsum_sums_int8 - AssertionError:
FAILED core/tests/test_einsum.py::TestEinsum::test_einsum_sums_uint8 - AssertionError:

which I would bet has something to do with SIMD.

@rgommers
Copy link
Member

The int8/uint8 einsum tests already have a test skip for macOS x86-64, which I had to add when switching to Meson. That job uses Clang too. On arm64 it passes, so seeing this it's very likely to be a Clang + x86 + exact compile flags used bug. Tracked in gh-23981, I'll edit the relevant comment there to point here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants