-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
BUG: Build failure (of 1.26.2) on SapphireRapids (avx512_spr
) due to multiple definition of avx512_qsort
and avx512_qselect
#25274
Comments
@branfosj thanks for reporting. I am taking a look. |
This looks like a bug in the build system. The issue seems to be that
|
ping @seiko2plus |
|
The behavior observed is not a bug but a consequence of defining a non-inline function in the C++ header files by |
@seiko2plus I concluded the same in intel/x86-simd-sort#111 (comment) TLDR: Indeed an
Having all functions have internal linkage fix that
And finally I think in both numpy and x86-simd-sort the use of templates and explicit specializations is overused or even misused. A common patter seems to be:
Why are those templates and not simply overloaded functions? That would make e.g. the above addition of |
#25376 should fix this build issue. Could you please verify? |
I checked out the PR locally and did a However I still think this is an ODR violation from numpy in linking together functions compiled with different architecture flags which may lead to runtime crashes depending on the linker and target environment/cpu |
That's true, but each compilation exports symbols with unique suffixes based on compiler # definition e.g.
If the compiler fails to inline a function then the priority goes to the lowest interest target that's why we tend to export unique weak symbols for each TPU, see #25045 (comment) for more clearfiction.
Both |
But the suffixing is not exhaustive as can be seen by this issue: This applies similar to most functions in |
That's because you are raising the ceiling of the baseline features. During the loading of the NumPy module, there is a validation step that raises a Python runtime error if the running machine does not support the baseline features, in order to avoid illegal instruction errors.
Let's differentiate between two situations: the weak symbols, which occur when the C++ compiler fails to inline inline-functions. In this case, the linker silently selects the first duplicated symbol. The second situation involves global symbols, which occur when the C++ compiler fails to inline non-inline functions (our issue here). This leads to a link-time error if there are any duplicated symbols. To deal with the possibility of duplicated weak symbols, the current approach is safe as long as SIMD kernels have unique symbols and non-suffixed functions of the lowest interest are chosen. Regarding duplicated global symbols, we shouldn't encounter that issue if we adhere to the standard. Moreover, such duplications can be detected at build-time. |
re-opened this issue till the backport of #25376 gets merged. |
Didn't you merge this 2 minutes prior which is why this got closed? |
Yes, I forgot to unlink it, however, since this issue relates to 1.26.x, I thought it would be better to leave it open till the backport and also the confirmation from the author of the issue. |
Ok, so you meant until that is merged into 1.26.x FWIW: I already made backport patches for 1.25.1 that also apply on 1.26.2:
@branfosj Confirmed that those solve the issue on his system too. |
Backported by #25475, thank you everyone. |
Describe the issue:
Building 1.26.2 on SapphireRapids with
or, on IceLake with
Fails due to multiple definition of
void avx512_qsort<_Float16>(_Float16*, long)
andvoid avx512_qselect<_Float16>(_Float16*, long, long)
.Reproduce the code example:
Error message:
Runtime information:
And the last part of the configure
Context for the issue:
No response
The text was updated successfully, but these errors were encountered: