Fix build with system's openmp #15369
Conversation
cmake 3.0.2-3.7 set OPENMP_FOUND cmake 3.9-3.15.0.rc2 set OpenMP_FOUND from cmake 3.9.5, OpenMP_<lang>_FOUND is preferred
|
Sounds less like a fix but rather a change in the logic if I'm not mistaken. It would be good to have performance numbers before we move forward with this PR. |
|
I'm afraid that this change is rather a complicated one due to long discussions that have not ended yet. You can find more info on the dev mailing list: [Discussion] Remove bundled llvm OpenMP. Initially the proposed change was in this PR #12160: Remove conflicting llvm OpenMP from cmake builds . We have already provided performance data on this, you can find and announcement on dev here and the document here. That was not enough, for reasons that are unclear to me, to convince everybody on validity and necessity of this change. Latest discussions happen in following dev threads: OMP and [VOTE] Remove conflicting OpenMP from CMake builds. @hubutui If you feel motivated enough to proceed with this change you can join the debate. |
|
@lebeg Hmm, actually, do user really care that much about performance affected by different OpenMP? As a packager's opinion, I want a good way to create this package. Maybe we could give user an option to choose the OpenMP, just like BLAS. |
|
Yes, it might be a good idea, but the numbers as stated in the performance doc don't differ that much. Managing this dependency within MXNet isn't a good idea at all in my opinion. |
|
It's been a long time since the discussion. I'll remove |
|
@mxnet-label-bot add [pr-awaiting-review] |
|
@hubutui would it be possible to add a CMake option to choose the OpenMP implementation that one wishes for? There's three options, MKL, 3rdparty OpenMP and the one found in the platform. So I think the pr goes in the good direction but we would need a commandline option. |
|
I don't think this is a good idea. we should standardize on one
implementation. Like I have pointed out, the default make build uses
llvm/intel version. cmake builds us llvm/intel version. if libgomp is
still being included, removing it should be investigated. Trying to
flip-flop between them on a whim is asking for trouble, and currently I
know of no advantage to using libgomp.
…On Mon, Jul 1, 2019 at 5:33 PM Pedro Larroy ***@***.***> wrote:
@hubutui <https://github.com/hubutui> would it be possible to add a CMake
option to choose the OpenMP implementation that one wishes for? There's
three options, MKL, 3rdparty OpenMP and the one found in the platform. So I
think the pr goes in the good direction but we would need a commandline
option.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#15369>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACVWZ7NBXS4I7C74P3GQNADP5KO55ANCNFSM4H3PNOZQ>
.
|
|
@cjolivier01 #9686 suggests transitioning fully to cmake, it would help a lot. Maybe dropping the Makefile someday? |
|
Using cmake is another subject altogether. I think you're missing the point. |
|
This is basically a duplicate of turning off llvm openmp. |
|
But we are distributing pip wheels with libgomp, at least we should be able to replicate that for development purposes. |
|
the reason is that there is mismatch between cmake and make builds. pip is currently using make for building libmxnet. for now if you want to replicate that you can use make |
|
@szha understood. it would be beneficial to be able to replicate that with CMake by having a selector for the openmp lib until then. |
Description
This pr fix build with openmp by:
FindOpenMPmodule from cmakeChecklist
Essentials
Please feel free to remove inapplicable items for your PR.
This pr is a minor modification of
CMakeLists.txt, simply find openmp firstly in the system, and then build from source if not found.