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

[OpenBLAS] Set min TARGET #2431

Merged
merged 1 commit into from
Jan 27, 2021
Merged

[OpenBLAS] Set min TARGET #2431

merged 1 commit into from
Jan 27, 2021

Conversation

omus
Copy link
Contributor

@omus omus commented Jan 22, 2021

When attempting to use the BB of [email protected] on Apple Silicon under x86 I ran into:

...
DelimitedFiles  ───  0.084462 seconds

signal (4): Illegal instruction: 4
in expression starting at /Users/omus/Development/Julia/x86/latest/usr/share/julia/stdlib/v1.7/LinearAlgebra/src/blas.jl:94

signal (4): Illegal instruction: 4
in expression starting at /Users/omus/Development/Julia/x86/latest/usr/share/julia/stdlib/v1.7/LinearAlgebra/src/blas.jl:94

signal (4): Illegal instruction: 4
in expression starting at /Users/omus/Development/Julia/x86/latest/usr/share/julia/stdlib/v1.7/LinearAlgebra/src/blas.jl:94

signal (4): Illegal instruction: 4
in expression starting at /Users/omus/Development/Julia/x86/latest/usr/share/julia/stdlib/v1.7/LinearAlgebra/src/blas.jl:94

signal (4): Illegal instruction: 4
in expression starting at /Users/omus/Development/Julia/x86/latest/usr/share/julia/stdlib/v1.7/LinearAlgebra/src/blas.jl:94

signal (4): Illegal instruction: 4
in expression starting at /Users/omus/Development/Julia/x86/latest/usr/share/julia/stdlib/v1.7/LinearAlgebra/src/blas.jl:94

signal (4): Illegal instruction: 4
in expression starting at /Users/omus/Development/Julia/x86/latest/usr/share/julia/stdlib/v1.7/LinearAlgebra/src/blas.jl:94
/bin/sh: line 1: 89876 Segmentation fault: 11  JULIA_BINDIR=/Users/omus/Development/Julia/x86/latest/usr/bin /Users/omus/Development/Julia/x86/latest/usr/bin/julia -g1 -O0 -C "native" --output-ji /Users/omus/Development/Julia/x86/latest/usr/lib/julia/sys.ji.tmp --startup-file=no --warn-overwrite=yes --sysimage /Users/omus/Development/Julia/x86/latest/usr/lib/julia/corecompiler.ji sysimg.jl .

I also found that compiling [email protected] from source under x86 worked without issue. When building the OpenBLAS BB locally I discovered:

Supporting multiple x86_64 cpu models with minimum requirement for the common code being HASWELL

Which was the difference between my source build and the BB version. As haswell supports AVX while the M1 does not this seems to be the source of the issue.

I've set the target to nehalem and using a local BB I've validated that the problem no longer exists.

@omus omus requested a review from staticfloat January 22, 2021 06:00
O/OpenBLAS/common.jl Outdated Show resolved Hide resolved
@giordano
Copy link
Member

giordano commented Jan 22, 2021

Did you try to run Julia after setting the environment variable OPENBLAS_CORETYPE?

@staticfloat
Copy link
Member

I don't think OPENBLAS_CORETYPE will help here; it's not crashing in a kernel, it's crashing in generic routines, presumably because it's passing -mavx to gcc while compiling its "generic" code. While Nehalem is a fairly safe bet, I think it's best if we go as safe as possible and do TARGET=GENERIC.

O/OpenBLAS/common.jl Outdated Show resolved Hide resolved
O/OpenBLAS/common.jl Outdated Show resolved Hide resolved
@giordano
Copy link
Member

Note that

  1. you need to touch all the build_tarballs.jl files for v0.3.13 in order to actually rebuild them
  2. v0.3.13 must have julia_compat="1.7" (which is a good edit for point 1)

@omus
Copy link
Contributor Author

omus commented Jan 22, 2021

While Nehalem is a fairly safe bet, I think it's best if we go as safe as possible and do TARGET=GENERIC.

I agree. I figured I'd just change enough to get this passing to start with.

@omus
Copy link
Contributor Author

omus commented Jan 22, 2021

2. v0.3.13 must have `julia_compat="1.7"` (which is a good edit for point 1)

I'll make the change. Just for my own understanding julia_compat enforces that a lower version of Julia can't use this BB? It is unlikely this would be used in 1.6 at this point but what's the rational behind enforcing it?

O/OpenBLAS/common.jl Outdated Show resolved Hide resolved
@omus omus force-pushed the cv/min-target-openblas branch from e64a150 to 427408a Compare January 22, 2021 21:24
@staticfloat
Copy link
Member

It is unlikely this would be used in 1.6 at this point but what's the rational behind enforcing it?

It's so that the bounds get encoded in the registry so that Pkg tools know about the restrictions on these JLLs. This is useful for when we do things like try to build a new version of SuiteSparse, which needs an OpenBLAS_jll to link against, but if we're building a new version of SuiteSparse for Julia v1.6 that depends on features in OpenBLAS, we have to be careful to not build a SuiteSparse that implicitly depends on features only available in OpenBLAS v0.3.13; when it tries to be loaded within Julia 1.6, it will fail.

This isn't a concern for SuiteSparse and OpenBLAS in particular, because the ABI is quite slow-moving, but it's a huge issue for other stdlibs like libgit2, libcurl and libmbedtls. So we just like to be consistent everywhere, as much as we can with the stdlibs.

@omus omus changed the title [OpenBLAS] Set min TARGET=NEHALEM [OpenBLAS] Set min TARGET Jan 26, 2021
@giordano
Copy link
Member

I'm a bit lost here: is this good to go now?

@omus
Copy link
Contributor Author

omus commented Jan 27, 2021

I'm a bit lost here: is this good to go now?

Yes, should be good to merge at this point.

@giordano giordano merged commit 837ccf4 into master Jan 27, 2021
@giordano giordano deleted the cv/min-target-openblas branch January 27, 2021 17:09
@omus
Copy link
Contributor Author

omus commented Jan 27, 2021

Confirmed the OpenBLAS_jll 0.3.13+1 release fixes the original issue

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