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: try clang build #131296

Closed
wants to merge 5 commits into from
Closed

openblas: try clang build #131296

wants to merge 5 commits into from

Conversation

junghans
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label May 18, 2023
@carlocab carlocab changed the title openblas.rb: try clang build openblas: try clang build May 19, 2023
@carlocab
Copy link
Member

This won't work as-is, because gfortran will still try to link with libgomp, which we don't want.

We would either need to patch some Makefiles, or handle this in brew/superenv.

Thoughts here, @Homebrew/core? I'm personally in favour of this change, along with using libomp everywhere on macOS. This allows us to use Clang instead of GCC when OpenMP is needed, and avoids the difficulties caused by software that have C++ code but also use OpenMP. [*]

Additional context at #129100, #129648.

@carlocab
Copy link
Member

I guess, alternatively, we could try building with clang but also using libgomp with that for OpenMP, but I'm a little less clear on how to make that work. I imagine it's possible though.

@fxcoudert
Copy link
Member

What is the benefit? It would be adding a dependency, and requiring us to hack the build, but I do not clearly see the gain. Also, how does that impact dependents? Specially those that rely on the Fortran OpenMP, which isn't available in libomp (but is part of libgomp).

@junghans
Copy link
Contributor Author

Can we use flang?

@junghans junghans closed this May 19, 2023
@ywwry66
Copy link
Contributor

ywwry66 commented May 19, 2023

gfortran will still try to link with libgomp, which we don't want.

It doesn't seem like that with my local clang+libomp build:

libopenblasp-r0.3.23.dylib:
	/opt/homebrew/opt/openblas/lib/libopenblas.0.dylib (compatibility version 0.0.0, current version 0.0.0)
	/opt/homebrew/opt/gcc/lib/gcc/current/libgfortran.5.dylib (compatibility version 6.0.0, current version 6.0.0)
	/opt/homebrew/opt/libomp/lib/libomp.dylib (compatibility version 5.0.0, current version 5.0.0)
	/opt/homebrew/opt/gcc/lib/gcc/current/libquadmath.0.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.100.3)

What is the benefit?

Adding one more reason: libgomp is not fork safe on macOS, and will probably never be. See:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58378
OpenMathLib/OpenBLAS#2197 (comment)

Can we use flang?

LLVM flang is still WIP and not production ready.

@carlocab
Copy link
Member

I've outlined some benefits above, but to make it more explicit: currently, building OpenBLAS with GCC means that any formula, particularly ones with dependents, with C++ code effectively has to choose between using OpenBLAS or OpenMP. If you need to use both (which seems to be the case for suite-sparse), then you are stuck linking against libstdc++, which makes things very difficult for dependents.

Unless we can build suite-sparse with GCC but link with libc++ instead?

@cho-m
Copy link
Member

cho-m commented May 19, 2023

Unless we can build suite-sparse with GCC but link with libc++ instead?

I see recommendations for -nostdinc++ -nodefaultlibs and manually re-adding libraries, e.g. -lc++ -lc++abi -lm -lc -lgcc_s -lgcc

Or maybe rebuilding gcc with -stdlib=libc++ support enabled like option in MacPorts (https://github.com/macports/macports-ports/blob/master/lang/gcc-devel/Portfile#L269-L281).


I guess, alternatively, we could try building with clang but also using libgomp with that for OpenMP

Not sure if this is possible. You can force linkage with LLVM Clang's -fopenmp=libgomp, but it doesn't seem to enable OpenMP. Maybe someone else knows of a way.


along with using libomp everywhere on macOS.

I think Conda Forge has done something similar. Not familiar with reading their recipes, but they may have drop-in replaced libgomp with libomp inside their gfortran.

Ref: https://conda-forge.org/docs/maintainer/knowledge_base.html#switching-openmp-implementation
Ref: https://github.com/conda-forge/gfortran_impl_osx-64-feedstock/pull/11/files

@github-actions github-actions bot added the outdated PR was locked due to age label Jun 19, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosquash Automatically squash pull request commits according to Homebrew style. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants