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

[finufft] match LLVM version of LLVMOpenMP #9423

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

jkrimmer
Copy link
Contributor

This PR fixes wrong test results in FINUFFT.jl on MacOS (see ludvigak/FINUFFT.jl#64).

This PR fixes wrong test results in FINUFFT.jl on MacOS (see ludvigak/FINUFFT.jl#64)
@giordano
Copy link
Member

Can you please elaborate on why this is necessary? It isn't immediately obvious to me.

@jkrimmer jkrimmer changed the title fix: match LLVM version of LLVMOpenMP [finufft] match LLVM version of LLVMOpenMP Sep 13, 2024
@jkrimmer
Copy link
Contributor Author

For whatever reason, the finufft library provided by finufft_jll yields wrong results on an M3 Macbook using a more recent LLVM version if more than ten threads are used (by default, finufft uses all available CPU cores, i.e., twelve threads on my M3 Pro). Accordingly, the tests performed by FINUFFT.jl fail. Unfortunately, the CI cannot reproduce this behavior since the Github runners do not use more than three or four threads.

@ludvigak
Copy link
Contributor

Nice catch, although a bit disturbing. What could be causing this?

@giordano
Copy link
Member

@jkrimmer just to be clear, you have verified this change solves your issue? I'm still very confused about what could be the cause of this.

@jkrimmer
Copy link
Contributor Author

Unfortunately, I have absolutely no idea what could be causing this issue!
I have tested this change locally and can confirm that the wrong results occur with all llvm versions ≥ v15. Hence, for the sake of consistency, I have chosen v13 to match the version of LLVMOpenMP.

@giordano
Copy link
Member

I wonder if this is related to #9265 (CC @awietek). The symptoms are a bit different (segmentation fault there, instead of wrong results), but llvmopenmp is the common theme.

Anyway, thanks for testing this!

@giordano giordano merged commit 1ec2082 into JuliaPackaging:master Sep 13, 2024
23 checks passed
@giordano
Copy link
Member

What confuses me a lot is that llvmopenmp on aarch64-darwin does have a dependency on the specific version of the compiler used to build it, because we need compiler-rt, but that's statically linked, so I'm missing what should be the correlation with the compiler used to compile other code.

@jkrimmer
Copy link
Contributor Author

Although I do not know if this issue is related to #9265, there is another problem with finufft on MacOS (at least on aarch64) that leads to segmentation faults in a threaded environment, see also ludvigak/FINUFFT.jl#63.

Regarding the LLVM version for building finufft, I assume there have been improvements in Apple Silicon support in LLVM v15 that might lead to the erroneous behavior in combination with OpenMP. The wrong test results also do not occur if I build finufft locally with Apple clang version 15.0.0 (clang-1500.3.9.4). Honestly, my choice of using v"13.0.1+1" in particular is a bit hand-waving.

@giordano
Copy link
Member

Note that Apple's clang-1500.3.9.4 is actually upstream LLVM 16: https://en.wikipedia.org/wiki/Xcode#Xcode_15.0_-_(since_visionOS_support)_2

@jkrimmer
Copy link
Contributor Author

Thanks for the reminder; however, I have tested all LLVM versions in between v13 and v17. Thereby, I observed the issue with the Bootstrap images for v15, v16, and v17.

@jkrimmer jkrimmer deleted the match_openmp_llvm branch September 19, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants