Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

libiomp 20150401 #38740

Closed
wants to merge 1 commit into from
Closed

libiomp 20150401 #38740

wants to merge 1 commit into from

Conversation

tschoonj
Copy link
Contributor

version bump
header installation path changed to prefix/include

see also discussion in #38707

@xu-cheng
Copy link
Member

I think you need to update the test case in clang-omp as well. Also, from the backward compatibility aspect, maybe create a symlink to the original place? Will let the other maintainers decide it.

@tschoonj
Copy link
Contributor Author

Ok, done. Will create a pull-request for clang-omp as soon as this is accepted

@tschoonj
Copy link
Contributor Author

@xu-cheng ...and the test failed because the clang-omp test doesn't work anymore.

Any suggestions? the pull request for clang-omp will also fail because it will still use the old libiomp...

@xu-cheng
Copy link
Member

Ok, done. Will create a pull-request for clang-omp as soon as this is accepted

You can update clang-omp in this PR.

@tschoonj
Copy link
Contributor Author

Nice, it worked!

@tdsmith
Copy link
Contributor

tdsmith commented Apr 18, 2015

If I want to build something against OpenMPI with gcc while I have clang-omp installed, does having this header directly in HOMEBREW_PREFIX/include cause trouble for my gcc build? Ping @metacollin.

@ianml
Copy link
Contributor

ianml commented Apr 23, 2015

@tdsmith I haven't tested it out yet but it looks like only bin is linked, the headers are in libexec. e: Whoops, wrong formula. In this case omp.h is in the libiomp subdirectory.

@tschoonj
Copy link
Contributor Author

@xu-cheng How about pulling this one in? Thanks!

@xu-cheng
Copy link
Member

@tschoonj

  • I would like to create symlink in old directory to keep backward compatibility.
  • I'm not quite familiar to OpenMP and its relationship with different compilers. So will let @tdsmith decide whether we should merge this.

@tdsmith
Copy link
Contributor

tdsmith commented Apr 28, 2015

Oh, I see! This looks okay; I didn't notice the -I#{include} in the test. (Sorry, I've been afk for the last week.)

Ed: No, I was confused. I'll see what the bottle looks like when it's rebuilt.

@tdsmith
Copy link
Contributor

tdsmith commented Apr 28, 2015

@BrewTestBot test this please

@xu-cheng
Copy link
Member

xu-cheng commented May 8, 2015

Ping @tdsmith

@tdsmith
Copy link
Contributor

tdsmith commented May 8, 2015

Oh, I let the damn build expire again, shame on me.

@BrewTestBot test this please

@xu-cheng
Copy link
Member

xu-cheng commented May 8, 2015

@BrewTestBot test this please

@xu-cheng
Copy link
Member

xu-cheng commented May 9, 2015

Ping @tdsmith in case you forget again. 😃

@tdsmith
Copy link
Contributor

tdsmith commented May 9, 2015

Thanks 😅

This does install a HOMEBREW_PREFIX/include/omp.h. It does not cause problems with gcc's omp implementation in the default prefix, because:

#include <...> search starts here:
 /usr/local/Cellar/gcc/4.9.2_1/lib/gcc/4.9/gcc/x86_64-apple-darwin14.0.0/4.9.2/include <-- omp.h is here
 /usr/local/include
 ...

and -I/usr/local/include is always ignored. But if I add e.g. -I/Users/tim/.brew/include, it will not be ignored, and the libiomp omp.h will be found by gcc instead of gcc's omp.h.

I would like to not put this omp.h in the default search path. Can we teach clang-omp an additional default header search path?

@xu-cheng
Copy link
Member

xu-cheng commented May 9, 2015

I think there is a talking that we should vendor libiomp inside clang-omp and boneyard this formula. It seems libiomp is only used by clang-omp. See #38707.

@xu-cheng
Copy link
Member

xu-cheng commented May 9, 2015

Also FWIW, I would like to have clang-omp supported in homebrew compiler list. So if a formulaneed :openmp, we will choose clang-omp instead of gcc to avoid libc++ and libstdc++ incompatible problem.

@metacollin
Copy link
Contributor

Sorry, been busy :(.

Custom include paths must be added by setting the precompiler define C_INCLUDE_DIRS to the paths. C_INCLUDE_DIRS is searched after the default search paths, so we can simply add whereever libiomp's include directory lives. One can find the actual #define near the top of llvm/tools/clang/include/clang/Config/config.h (It's the second or third define I believe).

Also, according to http://clang.llvm.org/get_started.html , cmake is the standard and preferred build system (I am not sure if this changed recently or its said that for a while and I never noticed heh). The cmakelists for clang will safely insert whatever we want into the config.h file, we simply pass -DC_INCLUDE_DIRS="#{homebrew_path_to_extra_include_dirs}" when calling cmake, and we're good to go. I've tested this and it works correctly. I am actually not too familiar with autotools, so if anyone knows a safe way to set that define without switching to cmake, the define we want is C_INCLUDE_DIRS.

Otherwise, we might consider switching the build system to cmake, which shouldn't be that big a problem.

Also, my personal opinion is that libiomp should remain a separate formula. It's a runtime library, one does not actually need it to build clang-omp, or even compile OpenMP aware programs, it is only needed to execute them. Intel releases new versions of the runtime fairly often, often with bug fixes and performance improvements, and it's an open OpenMP API runtime project, and can be used by any compiler, and should be a separate formula if only to make it available to developers using homebrew that are interested in adding OpenMP support to a compiler. And there is potential for other things to use the runtime, even if the only forumla currently using it is clang-omp.

But that's just my opinion, either way it's not going to be terrible if its separate, or terrible if its rolled in as a resource block in the clang-omp formula either. Just my two satoshis on the subject.

@metacollin
Copy link
Contributor

D'oh, its done with --with-c-include-dirs=. 😐 🌴 (face palm).

Forget about the cmake stuff heh. For being so easy, it sure isn't documented anywhere in the clang online docs. I discovered it looking through the configure script.

I am going to do a pull request momentarily, I'm waiting for clang to rebuild to double check that this actually does what it's supposed to. I'm on a 2008 Mac Pro, so clang takes a while to build =/.

@tdsmith
Copy link
Contributor

tdsmith commented May 9, 2015

Neat; Formula["libiomp"].opt_include/"libiomp" is probably the path we want to add.

Is there a corresponding option for linking libiomp5.dylib?

@metacollin
Copy link
Contributor

tdsmith, yep, that's what I am testing as we speak!

As for linking libiomp5, the name is unique, nothing will link to it unless it means to. If it were called libomp, it might be a problem, but there is one and only one library with the 'i' in front of the omp, and that's intel's runtime, with i signifying intel. Regardless, I am looking for info on how to add a custom lib search path to clang, but haven't found anything yet.

@tdsmith
Copy link
Contributor

tdsmith commented May 9, 2015

That makes sense; let's not worry about hiding the library. I think it could be helpful if Homebrew is installed to a prefix other than /usr/local but people who do that should know how to use LDFLAGS.

@xu-cheng
Copy link
Member

clang-omp has been updated. @tschoonj Please update this accordingly. Thanks.

@tschoonj
Copy link
Contributor Author

Nothing needs an update I think. I just tested it on my machine with the new clang-omp. Perhaps trigger a rebuild and see what happens?

@xu-cheng
Copy link
Member

You need to rebase and remove the head file installation location change

@tschoonj
Copy link
Contributor Author

this ok?

@@ -35,7 +35,7 @@ def install
system "cmake", ".", *args
system "make", "all", "common"

(include/"libiomp").install Dir["exports/common/include/*"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change this. It should be installed in include/libiomp now.

version bump
@tschoonj
Copy link
Contributor Author

Ok, done

@metacollin
Copy link
Contributor

Uh, please ignore my deleted posts. I completely misunderstood what was going on. Excited for this update :D Thanks tschoonj.

@xu-cheng
Copy link
Member

Thanks for the pull request! 🎉 Homebrew depends on contributions from community members like you and we're grateful for your support.

@xu-cheng xu-cheng closed this in 71869be May 11, 2015
@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants