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_jll] Optimial microarchitecture not downloaded #4252

Closed
ludvigak opened this issue Jan 14, 2022 · 6 comments · Fixed by #4669
Closed

[finufft_jll] Optimial microarchitecture not downloaded #4252

ludvigak opened this issue Jan 14, 2022 · 6 comments · Fixed by #4669
Labels
bug 🐛 Something isn't working

Comments

@ludvigak
Copy link
Contributor

We just added microarchitecture expansion to finufft_jll (#4249). However, when I try it out through FINUFFT.jll, it doesn't load the optimal artifact.

Specifically, it downloads x86_64-linux-gnu-march+x86_64 even though my computer can use x86_64-linux-gnu-march+avx2 (which is faster).

If I remove the x86_64 entry from Artifacts.toml, then it downloads the expected avx2 artifact and I get the expected speedup, so I know that it should work.
(ping @giordano)

@giordano
Copy link
Member

CC @staticfloat

@giordano giordano added the bug 🐛 Something isn't working label Jan 14, 2022
@giordano
Copy link
Member

Ok, talking with Elliot, it turns out the download side of the microarchitecture stuff isn't there yet. As I said, we haven't used it yet into the wild, so some things definitely need to be fleshed out. The good news is that at least we download the generic binaries by default, which is...well... no worse than before 😅

We need to "augment the platform" with the information of the microarchitecture by setting the tag p["march"] = first(host_isas[idx]). JuliaPackaging/BinaryBuilder.jl#1128 is developing right now the mechanism to do the platform augementation.

For our own record, this is code to get the ISA label for the current CPU:

julia> using Base.BinaryPlatforms

julia> host_arch = arch(HostPlatform())
       host_isas = CPUID.ISAs_by_family[host_arch]
       idx = findlast(((name, isa),) -> isa <= CPUID.cpu_isa(), host_isas)
       first(host_isas[idx])
"haswell"

Unfortunately it'll take still some time to make this work, but it's definitely something we want to eventually use, since we already have the infrastructure to build different microarchitectures!

@ludvigak
Copy link
Contributor Author

Great that you guys are working on it! We'll be ready to have it tested in the wild once you're done =)

@giordano
Copy link
Member

giordano commented Mar 23, 2022

It took us a few iterations and few mistakes in BinaryBuilder, but we should finally be there! ]add finufft_jll should get you the right binary for your microarchitecture:

julia> using finufft_jll

julia> finufft_jll.host_platform
Linux x86_64 {cxxstring_abi=cxx11, julia_version=1.7.2, libc=glibc, libgfortran_version=5.0.0, libstdcxx_version=3.4.29, march=avx2}

Note the march=avx2 property.

@ludvigak
Copy link
Contributor Author

ludvigak commented Apr 1, 2022

I noticed that it currently works great, but only if I force it to use the yanked release of finufft_jll.
Was there an issue with the deployment on avx512 architectures? (Do note that there is no avx512 artifact, since the current compiler doesn't seem to do a great job of that architecture.)

@giordano
Copy link
Member

giordano commented Apr 1, 2022

Was there an issue with the deployment on avx512 architectures? (Do note that there is no avx512 artifact, since the current compiler doesn't seem to do a great job of that architecture.)

Yes, the problem was that on AVX512 systems the choice of the artifact was falling back on the generic one, just because the package manager chooses the last artifact in the list, matching the architecture: the artifacts are currently written out sorted alphabetically, and so x86_64 was the last one. I was trying to do the same for #4675, but bumped into other issues, to be addressed by JuliaPackaging/BinaryBuilder.jl#1194 and JuliaPackaging/BinaryBuilderBase.jl#233, but the review of the latter required to do more changes, which unfortunately I don't have the time to do now 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants