[WIP] refactor OpenMP module; fixes #247

[bast]

Changes:

• append flags if either OPENMP_FOUND OR OpenMP_${_lang}_FOUND
• do not set OPENMP_FOUND
• make version dependence of workaround for CMake < 3.5 more explicit
• do not add_definitions(-DHAVE_OPENMP)
• rename to omp-flags.cmake to be more explicit and to not silently
  drop -DHAVE_OPENMP

Question: Unsure about the removal of -DHAVE_OPENMP but I am not sure what that variable means if OpenMP support is not detected for all project languages. Alternative would be to add a definition for each language separately. Yet another alternative would be to add the definition to compile flags. I am worried to break client codes since we do not have a functioning versioning and changelog in place.

[bast]

The solution has been proposed and discussed in #247. @loriab @robertodr
WIP until we answer above question(s).

[loriab]

It’d be nice if the kit ware findopenmp let one specify wanted langs as components since I don’t usually care if Fortean omp found.

Convenience wise, I’d prefer if this module continued setting the preprocessor definition.

How new is foreach in cmake?

[bast]

I agree about your comment on FindOpenMP.cmake. Foreach is old so no danger there.

Should this module set HAVE_OPENMP if at least one of the project languages has OMP support detected? Or should we define HAVE_${lang}_OPENMP (instead or in addition)?

[loriab]

This is where changes spill over from the build system to the code. I'd consider renaming HAVE_OPENMP more disruptive than where the openmp_flags go among the cmake vars. For the most part codes assume all or nothing wrt openmp enabled, I think.

Psi-centric, I'd just want HAVE_OPENMP back if CXX omp detected.

[robertodr]

I would still prefer this to spill over to the Autocmake client. Since we (the maintainers we) do not have a consensus on this issue yet, let's chug along with setting definitions in the module.

If we do add_definitions(-DHAVE_OPENMP), then it might very well happen that flags for one language are not found, but OpenMP specific code is still sent to the compiler by the preprocessor. Hopefully this crashes compilation, possibly it generates complete nonsense library/executable.

I suggest to loop over enabled languages only (in this way we skip the requirement for a Fortran compiler to use this module) and set HAVE_OPENMP if and only if OpenMP_<LANG>_FOUND is TRUE for all of them.

[loriab]

I don't particularly care, as I'll probably stick with the file in psi4/psi4#1017. The suggestion in @robertodr's 3rd paragraph, while nominally very sensible, would bring psi back to the original complaint in #247 of failed omp detection for enabled fortran effectively turning off omp throughout.

[bast]

Yes, we should only check enabled languages but still solve the issue.

I would like to give some feedback to the caller on whether something was found or not but am unsure how. The global HAVE_OPENMP is not elegant as we know and also ill-defined if not all languages have been found. If we simply remove it, some Autocmake client codes will become slower and developers may not notice.

Also unsure whether we should stop the code if I want --omp but some language is not found.

One option is that we return variables containing flags. And then the caller can add them globally or to a target only. Also the caller can verify whether the string is empty. If non-empty, the caller can add definitions to some target.

[loriab]

By the way, I started sketching out how I'd want math detection to properly interact with the new capabilities of FindOpenMP.cmake, with the primary complication being that compiler/blas distribution/omp are all tangled together. What I'm settling on is adding a third pass to services (OMP, BLAS, LAPACK), where OMP_LIBS is by default empty (and stays that way for most BLAS dists). It serves as a bridge between math and whatever OpenMP::OpenMP_CXX (for example) properly provides. So MKL_BLAS_LIBS can be just mkl_rt, OpenMP::OpenMP_CXX supplies gomp;pthreads, and MKL_OMP_LIBS is iomp5;-Wl,--as-needed. Target-wise, lapack depends on lapk blas omp, and omp depends on run-time detected OpenMP::OpenMP_CXX.