Open paciorek opened 5 years ago
In the parallel_eigen branch, I've inserted -fopenmp
in nimble/inst/make/*.in
.
I don't think we need to insert -fopenmp
in src/Makevars*in
or inst/include/CppCode/Makevars
as I don't think we have any Eigen-based linear algebra in our hard-coded C++, but @perrydv let me know if you have other thoughts.
OTOH, there is presumably no downside to enabling -fopenmp
for {lib,}nimble.so
so we might do it just in case there is some such code in the future.
One other comment is that Eigen's parallelization seems limited. As of Eigen 3.3.5:
Currently, the following algorithms can make use of multi-threading:
general dense matrix - matrix products
PartialPivLU
row-major-sparse * dense vector/matrix products
ConjugateGradient with Lower|Upper as the UpLo template parameter.
BiCGSTAB with a row-major sparse matrix format.
LeastSquaresConjugateGradient
So turning on parallelization will probably not help end users by and large.
Testing indicates an issue with -fopenmp
on macOS.
Also not clear that -fopenmp
would by default be supported on Windows.
Writing R extensions section 1.2.11 indicates one should use SHLIB_OPENMP_CXXFLAGS in Makevars* files.
I've now implemented this, but on one of my Macs, -fopenmp is being injected despite seeming to not be supported. Perhaps this means R was configured incorrectly (though I think I did a basic R install on that machine). But we should be cautious here for fear of breaking users' use of nimble if they don't have openMP support.
Side note: users can also do this at run time if their compiler supports openMP:
Sys.setenv("PKG_CXXFLAGS"="-fopenmp")
Sys.setenv("PKG_LIBS"="-fopenmp")
Cautionary note from Writing R extensions: Note that the macro SHLIB_OPENMP_CXXFLAGS applies to the default C++ compiler and not necessarily to the C++11/14/17 compiler: users of the latter should do their own configure checks (an example is available in CRAN package ARTP2).
Ok, so this is failing on two separate Macs. -fopenmp is being injected into the clang++ call and that is erroring out with an "unsupported option '-fopenmp'" error.
This thread seems to be relevant, but I'm not understanding everything there (in particular the discussion is really about Travis). https://github.com/jonclayden/mmand/issues/4
On SCF Mac, SHLIB_OPENMP_CXXFLAGS seems to be set to "-fopenmp" in:
/Library/Frameworks/R.framework/Versions/3.4/Resources/etc/Makeconf
Unfortunately if one then sets the following in a local Makevars:
PKG_CPPFLAGS= -DR_NO_REMAP -Wno-misleading-indentation -Wno-ignored-attributes -Wno-deprecated-declarations
PKG_CXXFLAGS=$(SHLIB_OPENMP_CXXFLAGS)
PKG_LIBS= $(SHLIB_OPENMP_CXXFLAGS) $(LAPACK_LIBS) $(BLAS_LIBS)
CXX_STD = CXX11
as per Nimble's inst/make/Makevars, then compilation fails.
Unclear why "-fopenmp" is set given openMP seemingly can't be successfully used.
My current thought is that at run-time we want to set -fopenmp if openMP is available to user via the local Makevars in nimble_generatedCode. Why?
1) it seems that SHLIB_OPENMP_CXXFLAGS could be set in a user's R such that it causes compilation failures. So we can't rely on the value it is set to. 2) Most Mac/Win users will get the binary package so we have no ability to monkey with nimble's inst/make/Makevars as it is placed on a user's machine. 2a) Any attempt to monkey with nimble's inst/make/Makevars is done at package build time and for Mac/Win users this is based on CRAN's build machine.
Plan - look at how dynamicRegistrations stuff is done and how we write Makevars in nimble_generatedCode. We might set a nimbleOption the first time compilation is done based on querying the user's system to see if -fopenmp can be used and then at subsequent times in a given R session, use the nimbleOption.
Not clear on the following:
It would probably be good to allow for the nimbleOption to behave such that users could turn on or off -fopenmp as they prefer and we won't override that. So the options might be: auto/on/off where auto is that we detect. So there might be a user-facing option and then the option that actually says whether openMP is on for a given user session so we don't have to keep testing whether -fopenmp works.
We should do this for next release.